| Message ID | 1349868762-10021-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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; }
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
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
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.
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
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
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).
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.
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);
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(-)