Re: [libvirt] [PATCH 14/15] util: maintain caps when running command with uid != 0
On 02/08/2013 02:05 PM, Eric Blake wrote:
On 02/07/2013 02:37 PM, Laine Stump wrote:
> virCommand was previously calling virSetUIDGID() to change the uid and
> gid of the child process, then separately calling
> virSetCapabilities(). This did not work if the desired uid was != 0,
> since a setuid to anything other than 0 normally clears all
> capabilities bits.
>
> The solution is to use the new virSetUIDGIDWithCaps(), sending it the
> uid, gid, and capabilities bits. This will get the new process setup
> properly.
>
> Since the static functions virSetCapabilities() and
> virClearCapabilities are no longer called, they have been removed.
>
> NOTE: When combined with "filecap $path-to-qemu sys_rawio", this patch
> will make CAP_SYS_RAWIO (which is required for passthrough of generic
> scsi commands to a guest - see commits e8daeeb, 177db08, 397e6a7, and
> 74e0349) be retained by qemu when necessary. Apparently that
> capability has been broken for non-root qemu every since it was
s/every/ever/
> originally added.
> ---
> src/util/vircommand.c | 76 ++++++---------------------------------------------
> 1 file changed, 8 insertions(+), 68 deletions(-)
ACK.
> -# else
> -static int virClearCapabilities(void)
> -{
> -// VIR_WARN("libcap-ng support not compiled in, unable to clear "
> -// "capabilities");
Odd that we had commented this out previously.
1) Even more odd is that this function wasn't being called from
anywhere, but still existed *and* didn't cause a compile error. I had
thought that unreferenced static functions always caused a warning
(which we promote to an error).
2) There are several places where we call virCommandClearCapabilities()
without regard to whether or not WITH_CAPNG is actually set. Systems
without WITH_CAPNG would have tons of warnings, which I'm guessing
people don't want. If we want to do a warning or error, maybe we could
do it at configure time (maybe require people to specifically say
"--without-capng" or something).
Should patch 13/15 log
any warnings when we are not preserving/clearing capabilities, rather
than silently ignoring the capability request?
>
> - if (cmd->uid > 0 || cmd->gid > 0) {
> - VIR_DEBUG("Setting child uid:gid to %u:%u", cmd->uid,
cmd->gid);
> - if (virSetUIDGID(cmd->uid, cmd->gid) < 0)
> + /* The steps above may need todo something privileged, so we delay
As long as you are touching this comment, s/todo/to do/ (but you've
moved it at least twice in this series, so it depends on how much churn
you want on when you finally fix it).
Okay. I'll do it here.