[libvirt] [PATCH] Call initgroups for qemu's uid prior to exec

[Laine Stump]

On 12/21/2010 04:52 PM, Eric Blake wrote:
> On 12/21/2010 01:45 PM, Laine Stump wrote:
>> This patch fixes https://bugzilla.redhat.com/show_bug.cgi?id=664406
>>
>> If qemu is run as a different uid, it has been unable to access mode
>> 0660 files that are owned by a different user, but with a group that
>> the qemu is a member of (aside from the one group listed in the passwd
>> file). initgroups will change the group membership of the process (and
>> its children) to match the new uid.
>> ---
>>   src/qemu/qemu_security_dac.c |   27 +++++++++++++++++++++++++++
>>   1 files changed, 27 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/qemu/qemu_security_dac.c b/src/qemu/qemu_security_dac.c
>> index 55dc0c6..2e60aec 100644
>> --- a/src/qemu/qemu_security_dac.c
>> +++ b/src/qemu/qemu_security_dac.c
>> @@ -12,6 +12,8 @@
>>   #include<sys/types.h>
>>   #include<sys/stat.h>
>>   #include<fcntl.h>
>> +#include<pwd.h>
>> +#include<grp.h>
>>
>>   #include "qemu_security_dac.h"
>>   #include "qemu_conf.h"
>> @@ -558,6 +560,30 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
>>           }
>>       }
>>       if (driver->user) {
>> +        struct passwd pwd, *pwd_result;
>> +        char *buf = NULL;
>> +        size_t bufsize = 16384;
> qemu_driver.c sets this to 1024*1024.  Will that matter?

The usage in qemu_driver.c re-uses a buffer that was already 
conveniently allocated for something else. This buffer is used to 
contain all the strings encountered in a single entry from /etc/passwd. 
The manpage for getpwuid_r() has example code to learn the maximum 
possible size for all the strings in one line, but that example falls 
back to 16384 if it fails to learn the system limit, saying "this should 
be big enough for anything".

>    For that
> matter, can't you provide this functionality in a single .c file so that
> both qemudOpenAsUID and qemuSecurityDACSetProcessLabel can share the
> benefits of common code?

If I put it in a separate function, I won't feel so bad about adding in 
the extra code to detect the absolute max size to allocate. Of course, 
if I put it in a util file, we'll have to worry about making it compile 
on all you mother's favorite platforms, so... :-)

> That refactoring probably deserves a v2.

Sure, I'll take a stab at it.

>> @@ -566,6 +592,7 @@ qemuSecurityDACSetProcessLabel(virSecurityDriverPtr drv ATTRIBUTE_UNUSED,
>>           }
>>       }
>>
>> +
>>       return 0;
> Spurious whitespace change.

Oops.