| Message ID | 20230602174806.2060683-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | linux-user: Return EINVAL for getgroups() with negative gidsetsize | expand |
Le 02/06/2023 à 19:48, Peter Maydell a écrit : > Coverity doesn't like the way we might end up calling getgroups() > with a NULL grouplist pointer. This is fine for the special case > of gidsetsize == 0, but we will also do it if the guest passes > us a negative gidsetsize. (CID 1512465) > > Explicitly fail the negative gidsetsize with EINVAL, as the kernel > does. This means we definitely only call the libc getgroups() > with valid parameters. > > Possibly Coverity may still complain about getgroups(0, NULL), but > that would be a false positive. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/syscall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 89b58b386b1..29fdfdf18e4 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -11574,7 +11574,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, > g_autofree gid_t *grouplist = NULL; > int i; > > - if (gidsetsize > NGROUPS_MAX) { > + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) { > return -TARGET_EINVAL; > } > if (gidsetsize > 0) { Reviewed-by: Laurent Vivier <laurent@vivier.eu>
On 2/6/23 19:48, Peter Maydell wrote: > Coverity doesn't like the way we might end up calling getgroups() > with a NULL grouplist pointer. This is fine for the special case > of gidsetsize == 0, but we will also do it if the guest passes > us a negative gidsetsize. (CID 1512465) > > Explicitly fail the negative gidsetsize with EINVAL, as the kernel > does. This means we definitely only call the libc getgroups() > with valid parameters. > > Possibly Coverity may still complain about getgroups(0, NULL), but > that would be a false positive. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/syscall.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
02.06.2023 20:48, Peter Maydell wrote: > Coverity doesn't like the way we might end up calling getgroups() > with a NULL grouplist pointer. This is fine for the special case > of gidsetsize == 0, but we will also do it if the guest passes > us a negative gidsetsize. (CID 1512465) > > Explicitly fail the negative gidsetsize with EINVAL, as the kernel > does. This means we definitely only call the libc getgroups() > with valid parameters. > > Possibly Coverity may still complain about getgroups(0, NULL), but > that would be a false positive. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> I especially kept the negative case like this when initially writing this code, thinking to return whatever error the kernel will return. But I guess it doesn't really matter here, and the less fragile corner cases, the better. Reviewed-by: Michael Tokarev <mjt@tls.msk.ru> Thanks, /mjt
02.06.2023 20:48, Peter Maydell wrote: > @@ -11574,7 +11574,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, > g_autofree gid_t *grouplist = NULL; > int i; > > - if (gidsetsize > NGROUPS_MAX) { > + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) { > return -TARGET_EINVAL; > } > if (gidsetsize > 0) { FWIW, there's another piece of code exactly like this one, for TARGET_NR_getgroups32. The same change is needed there too. Thanks, /mjt
03.06.2023 20:11, Michael Tokarev wrote: > 02.06.2023 20:48, Peter Maydell wrote: > >> @@ -11574,7 +11574,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, >> g_autofree gid_t *grouplist = NULL; >> int i; >> - if (gidsetsize > NGROUPS_MAX) { >> + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) { >> return -TARGET_EINVAL; >> } >> if (gidsetsize > 0) { > > FWIW, there's another piece of code exactly like this one, > for TARGET_NR_getgroups32. The same change is needed there too. Peter, will you respin this (to include getgroups32 case), or should I ? (The change is trivial enough to carry though -trivial@). Thanks, /mjt
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 89b58b386b1..29fdfdf18e4 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -11574,7 +11574,7 @@ static abi_long do_syscall1(CPUArchState *cpu_env, int num, abi_long arg1, g_autofree gid_t *grouplist = NULL; int i; - if (gidsetsize > NGROUPS_MAX) { + if (gidsetsize > NGROUPS_MAX || gidsetsize < 0) { return -TARGET_EINVAL; } if (gidsetsize > 0) {
Coverity doesn't like the way we might end up calling getgroups() with a NULL grouplist pointer. This is fine for the special case of gidsetsize == 0, but we will also do it if the guest passes us a negative gidsetsize. (CID 1512465) Explicitly fail the negative gidsetsize with EINVAL, as the kernel does. This means we definitely only call the libc getgroups() with valid parameters. Possibly Coverity may still complain about getgroups(0, NULL), but that would be a false positive. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- linux-user/syscall.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)