Patchwork virtfs-proxy-helper: check return code of setfsgid/setfsuid

login
register
mail settings
Submitter Paolo Bonzini
Date Oct. 10, 2012, 11:32 a.m.
Message ID <1349868762-10021-1-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/190615/
State New
Headers show

Comments

Paolo Bonzini - Oct. 10, 2012, 11:32 a.m.
Fixes the following error with glibc 2.16 on Fedora 18:

virtfs-proxy-helper.c: In function ‘setfsugid’:
virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
cc1: all warnings being treated as errors

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 fsdev/virtfs-proxy-helper.c | 8 ++++++--
 1 file modificato, 6 inserzioni(+), 2 rimozioni(-)
Stefan Weil - Oct. 10, 2012, 4:14 p.m.
Am 10.10.2012 13:32, schrieb Paolo Bonzini:
> Fixes the following error with glibc 2.16 on Fedora 18:
>
> virtfs-proxy-helper.c: In function ‘setfsugid’:
> virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
> virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
> cc1: all warnings being treated as errors
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>   fsdev/virtfs-proxy-helper.c | 8 ++++++--
>   1 file modificato, 6 inserzioni(+), 2 rimozioni(-)
>
> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
> index f9a8270..b34a84a 100644
> --- a/fsdev/virtfs-proxy-helper.c
> +++ b/fsdev/virtfs-proxy-helper.c
> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>           CAP_DAC_OVERRIDE,
>       };
>   
> -    setfsgid(gid);
> -    setfsuid(uid);
> +    if (setfsgid(gid) != 0) {
> +        return -1;
> +    }

Wouldn't setfsgid(gid) == gid be also ok?

I have no idea whether it is a realistic scenario to call
that function with a gid which is already set.


> +    if (setfsuid(uid) != 0) {
> +        return -1;
> +    }
>   

The same question applies here.

>       if (uid != 0 || gid != 0) {
>           return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);

The Linux manpage for both functions says something
completely different. Extract from manpage of setfsuid:

"On success, the previous value of fsuid is returned.
  On error, the current value of fsuid is returned."

In reality, the functions return 0 when called by root.

Regards

Stefan
Paolo Bonzini - Oct. 10, 2012, 4:17 p.m.
Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>
>>
>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>> index f9a8270..b34a84a 100644
>> --- a/fsdev/virtfs-proxy-helper.c
>> +++ b/fsdev/virtfs-proxy-helper.c
>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>           CAP_DAC_OVERRIDE,
>>       };
>>   -    setfsgid(gid);
>> -    setfsuid(uid);
>> +    if (setfsgid(gid) != 0) {
>> +        return -1;
>> +    }
> 
> Wouldn't setfsgid(gid) == gid be also ok?

Of course, it should be < 0.  I have no idea how to test this thing...

Paolo
Stefan Weil - Oct. 10, 2012, 4:23 p.m.
Am 10.10.2012 18:17, schrieb Paolo Bonzini:
> Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>>
>>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>>> index f9a8270..b34a84a 100644
>>> --- a/fsdev/virtfs-proxy-helper.c
>>> +++ b/fsdev/virtfs-proxy-helper.c
>>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>>            CAP_DAC_OVERRIDE,
>>>        };
>>>    -    setfsgid(gid);
>>> -    setfsuid(uid);
>>> +    if (setfsgid(gid) != 0) {
>>> +        return -1;
>>> +    }
>> Wouldn't setfsgid(gid) == gid be also ok?
> Of course, it should be < 0.  I have no idea how to test this thing...
>
> Paolo

< 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;
}

Stefan

#include <stdio.h>
#include <stdlib.h>
#include <unistd.h> /* glibc uses <sys/fsuid.h> */
#include <sys/fsuid.h>

int main(int argc, char *argv[])
{
   uid_t fsuid = strtoul(argv[1], 0, 0);
   int r = setfsuid(fsuid);
   printf("setfsuid(%u) returned %u\n", fsuid, r);
   return 0;
}
Paolo Bonzini - Oct. 10, 2012, 4:36 p.m.
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 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
Stefan Weil - Oct. 10, 2012, 4:54 p.m.
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
Stefan Weil - Oct. 10, 2012, 4:59 p.m.
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.
Paolo Bonzini - Oct. 10, 2012, 5 p.m.
Il 10/10/2012 18:54, Stefan Weil ha scritto:
>>
>>     if (setfsuid(uid) < 0 || setfsuid(uid) != uid) {
>>         return -1;
>>     }
>>
>> but it seems wasteful to do four syscalls instead of two.
> 
> I added a local variable in my example to avoid those extra
> syscalls.

Note that the two setfsuid() calls are different.

The first checks the "-1" error from glibc.  The second says "if the
first call succeeded, the second call should see "uid" as the current
fsuid and the second call will be a no-op; if not, the first call must
have failed".

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

So it would be

    if (setfsuid(uid) < 0) {
        return -1;
    }
    if (setfsuid(uid) != uid) {
        errno = EPERM;
        return -1;
    }

I still prefer my v2 (v1 is wrong).  The return path seems to be dead,
but it's not worse than before...

Paolo
Eric Blake - Oct. 10, 2012, 5:55 p.m.
On 10/10/2012 05:32 AM, Paolo Bonzini wrote:
> Fixes the following error with glibc 2.16 on Fedora 18:
> 
> virtfs-proxy-helper.c: In function ‘setfsugid’:
> virtfs-proxy-helper.c:293:13: error: ignoring return value of ‘setfsgid’, declared with attribute warn_unused_result [-Werror=unused-result]
> virtfs-proxy-helper.c:294:13: error: ignoring return value of ‘setfsuid’, declared with attribute warn_unused_result [-Werror=unused-result]
> cc1: all warnings being treated as errors
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  fsdev/virtfs-proxy-helper.c | 8 ++++++--
>  1 file modificato, 6 inserzioni(+), 2 rimozioni(-)

How does this differ from this earlier posting?
https://lists.gnu.org/archive/html/qemu-devel/2012-10/msg00851.html
Eric Blake - Oct. 10, 2012, 5:58 p.m.
On 10/10/2012 10:17 AM, Paolo Bonzini wrote:
> Il 10/10/2012 18:14, Stefan Weil ha scritto:
>>>
>>>
>>> diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
>>> index f9a8270..b34a84a 100644
>>> --- a/fsdev/virtfs-proxy-helper.c
>>> +++ b/fsdev/virtfs-proxy-helper.c
>>> @@ -290,8 +290,12 @@ static int setfsugid(int uid, int gid)
>>>           CAP_DAC_OVERRIDE,
>>>       };
>>>   -    setfsgid(gid);
>>> -    setfsuid(uid);
>>> +    if (setfsgid(gid) != 0) {
>>> +        return -1;
>>> +    }
>>
>> Wouldn't setfsgid(gid) == gid be also ok?
> 
> Of course, it should be < 0.  I have no idea how to test this thing...

POSIX states that uid_t and gid_t may be unsigned, so checking for < 0
is not necessarily possible (really, all you can check for is equality
with the same value as ((uid_t)-1) when put through integer promotion
rules).
Mohan Kumar M - Oct. 11, 2012, 7:25 a.m.
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.

Patch

diff --git a/fsdev/virtfs-proxy-helper.c b/fsdev/virtfs-proxy-helper.c
index f9a8270..b34a84a 100644
--- a/fsdev/virtfs-proxy-helper.c
+++ b/fsdev/virtfs-proxy-helper.c
@@ -290,8 +290,12 @@  static int setfsugid(int uid, int gid)
         CAP_DAC_OVERRIDE,
     };
 
-    setfsgid(gid);
-    setfsuid(uid);
+    if (setfsgid(gid) != 0) {
+        return -1;
+    }
+    if (setfsuid(uid) != 0) {
+        return -1;
+    }
 
     if (uid != 0 || gid != 0) {
         return do_cap_set(cap_list, ARRAY_SIZE(cap_list), 0);