qemu-trivial
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-trivial] [Qemu-devel] [PATCH] virtfs-proxy-helper: check retur


From: Stefan Weil
Subject: Re: [Qemu-trivial] [Qemu-devel] [PATCH] virtfs-proxy-helper: check return code of setfsgid/setfsuid
Date: Wed, 10 Oct 2012 18:59:18 +0200
User-agent: Mozilla/5.0 (X11; Linux i686; rv:15.0) Gecko/20120912 Thunderbird/15.0.1

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.





reply via email to

[Prev in Thread] Current Thread [Next in Thread]