bug on ppc (and potentially other arches) in setgroups.c

[Phil Estes]

On 07/02/2009 12:57 AM, Mike Frysinger wrote:
> On Wednesday 01 July 2009 22:44:58 Phil Estes wrote:
>    
>> Before I open a bug, I thought I would look for clarification of what I
>> believe I'm seeing in libc/sysdeps/linux/common/setgroups.c.  I think
>> it's possible that the code was originally i386-centric and has not been
>> updated as other architectures are added.
>>      
> it isnt i386 at all.  the 16 vs 32 bitness you note later on is correct, and
> not available in all kernels, hence the logic follows what has largely been
> the standard in practice:
>   - if the arch is 64bit, the [gs]etgroups functions have always been 32bit
>   - if the arch is 32bit:
>    - historically, only 16bit versions were available, thus no xxx32 means the
> wrapper is necessary
>    - with xxx32 version, 32bit was added, so the wrapper is not needed
>
>    

Ok, so that confirms what I assumed, however, it doesn't match the 
actual implementation of arch/powerpc in the kernel
>> Background:  the LTP has a testcase "setgroups04"--the purpose of which
>> is to try and get a -EFAULT out of the setgroups syscall by passing in a
>> known invalid memory value.  This should flow into the kernel and get an
>> -EFAULT after a failure of "copy_from_user" of the groups list pointer
>> passed to the syscall.
>>      
> if you want to make sure a test case flows into the kernel, LTP needs to use a
> syscall().  last time i reviewed this failure, i chalked the uClibc behavior
> up as acceptable as the standard does not dictate what must happen if the
> array specified is invalid with getgroups.  in my mind, that means a crash is
> acceptable behavior.  considering uClibc aims for small code, adding if()
> checks for stupid programmers when the checks are not required by POSIX is
> wrong -- let the code blow up.
>
> really the best thing to do would be to pose the question to the POSIX mailing
> lists for clarification (you should also ask about getgroups since the two
> should behave the same and setgroups is not in POSIX).
>
>    
Yup--point taken on LTP
>> In the case of uClibc on the ppc architecture, this means that the code
>> flows through the setgroups "wrapper" in setgroups.c which tries to copy
>> the passed-in group array to an array of allocated kernel_gid_t
>> structs.   In tracing back through why this might be, it seems to be
>> that this code would be for the compat syscall for 16-bit UIDs
>> (implemented in the kernel in uid16.c); and therefore the kernel gid_t
>> might not match the userspace gid_t; hence the copying.  It tries to
>> only use this wrapper if __NR_setgroups32 isn't defined, which it is not
>> for ppc.  But, on ppc, there is no 16-bit version or compat version of
>> the setgroups syscall; therefore, there should also be no logic to
>> attempt to perform the copying, which is really not useful on ppc (and
>> potentially other architectures which don't have this setgroups vs.
>> setgroups32 syscall issue).
>>      
> are you sure that this is the case ?  or are you assuming "no __NR_setgroups32
> means setgroups has always been 32bit" ?  if you can get verification from
> LKML, then we can look at changing uClibc.  this isnt a ppc32 specific issue
> as i see avr32, mips32, parisc32, and xtensa are in a similar boat.
> -mike
>    
I walked through this path with the ppc 4xx maintainer yesterday.. 
definitely __NR_setgroups on ppc is wired to the standard 32-bit UIDs 
syscall..there is no support for the old uid16.c implementations on ppc 
today.

Thanks,
Phil