[libvirt] [PATCH 14/15] util: maintain caps when running command with uid != 0

[Laine Stump]

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.