Re: [Qemu-trivial] [Qemu-devel] [PATCH] virtfs-proxy-helper: check return code of setfsgid/setfsuid

[Stefan Weil <sw@weilnetz.de>]

Am 10.10.2012 18:54, schrieb Stefan Weil:
> Am 10.10.2012 18:36, schrieb Paolo Bonzini:
>> Il 10/10/2012 18:23, Stefan Weil ha scritto:
>>> < 0 would be wrong because it looks like both functions never
>>> return negative values.
>>> I just wrote a small test program (see
>>> below) and called it with different uids with and without root
>>> rights. This pattern should be fine:
>>>
>>> new_uid = setfsuid(uid);
>>> if (new_uid != 0 && new_uid != uid) {
>>>    return -1;
>>> }
>> I didn't really care about this case.  I assumed that the authors knew
>> what they were doing...
>>
>> What I cared about is: "When glibc determines that the argument is not a
>>   valid  group  ID,  it will  return  -1  and set errno to EINVAL 
>> without
>> attempting the system call".
>
> I was not able to get -1 with my test program: any value which I tried
> seemed to work when the program was called with sudo.
>
>>
>> I think this would also work:
>>
>>     if (setfsuid(uid) < 0 || setfsuid(uid) != uid) {
>>         return -1;
>>     }
>>
>> but it seems wasteful to do four syscalls instead of two.
>>
>> Paolo
>
> I added a local variable in my example to avoid those extra
> syscalls.
>
> Your last patch v2 does not handle missing rights (no root)
> because in that case the functions don't return a value < 0
> but fail nevertheless.Calling a program which requires
> root privileges from a normal user account is usually a
> very common error. I don't know the use cases for virtfs -
> maybe that's no problem here.
>
> The functions have an additional problem: they don't set
> errno (see manpages). I tested this, and here the manpages
> are correct. The code in virtfs-proxy-helper expects that
> errno was set, so the patch must set errno = EPERM or
> something like that.
>
> Stefan

Maybe the author of those code can tell us more on the
use cases and which errors must be handled.

Is it necessary to use those functions at all (they are very
Linux specific), or can they be replaced by seteuid, setegid?

Regards

Stefan W.