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

["M. Mohan Kumar" <mohan@in.ibm.com>]

Stefan Weil <sw@weilnetz.de> writes:

We need to change fsuid and fsgid in 9p server side when 9p client wants
to create a file with given uid and gid.

In my case setfsuid and setfsgid never return -1 even if a normal user tries to
change fsuid.

I am running F17 and glibc is 2.15-56.fc17

IMHO setfsuid/setfsgid need to return -1 & set errno to EPERM when calling user
lacks CAP_SETUID capability. I can work on the kernel side to return
proper error values.

Also as per the man page:
       When glibc determines that the argument is not a valid user ID,
       it will return -1 and set errno  to  EINVAL
       without attempting the system call.

If it mean a nonexistent id by 'not a valid user ID' it may be a
problem in virtfs case. Client sends the user id details which may be
a nonexistent user id in host system. Ideally setfsuid setfsgid sets
whatever uid/gid passed if the caller is root as of now.   


> 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.