On Sat, Jun 01, 2024 at 02:24:03PM GMT, Alejandro Colomar wrote: > Hi Thomas, > > On Sat, Jun 01, 2024 at 01:05:02PM GMT, Thomas Weißschuh wrote: > > On 2024-06-01 11:31:56+0000, Alejandro Colomar wrote: > > > Since libc's prctl(2) wrapper is a variadic function, arguments must > > > have the right width. Otherwise, the behavior is undefined. > > > > Ack. > > > > > Also, the 5 arguments must be specified always, or the behavior is also > > > undefined. libc reads 5 values and passes them all to the kernel, so if > > > one is uninitialized, the kernel will receive garbagge, which could > > > result in EINVAL (most likely), or worse, a different action. > > > > This seems surprising. > > > > The kernel should only check the arguments it documents and not more. > > Hmmm, some prctl(2) calls don't document a need for passing 0 (probably > for legacy compatibility; you're right. Only newer prctl(2)s check > those args. > > And see for example these kernel commit: > > commit e9d1b4f3c60997fe197bf0243cb4a41a44387a88 > Author: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > Date: Thu Jan 8 14:30:22 2015 -0800 > > x86, mpx: Strictly enforce empty prctl() args > > Description from Michael Kerrisk. He suggested an identical patch > to one I had already coded up and tested. > > commit fe3d197f8431 "x86, mpx: On-demand kernel allocation of bounds > tables" added two new prctl() operations, PR_MPX_ENABLE_MANAGEMENT and > PR_MPX_DISABLE_MANAGEMENT. However, no checks were included to ensure > that unused arguments are zero, as is done in many existing prctl()s > and as should be done for all new prctl()s. This patch adds the > required checks. > > Suggested-by: Andy Lutomirski <luto@xxxxxxxxxxxxxx> > Suggested-by: Michael Kerrisk <mtk.manpages@xxxxxxxxx> > Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > Cc: Dave Hansen <dave@xxxxxxxx> > Link: http://lkml.kernel.org/r/20150108223022.7F56FD13@xxxxxxxxxxxxxxxxxx > Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx> > > diff --git a/kernel/sys.c b/kernel/sys.c > index a8c9f5a7dda6..ea9c88109894 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2210,9 +2210,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > up_write(&me->mm->mmap_sem); > break; > case PR_MPX_ENABLE_MANAGEMENT: > + if (arg2 || arg3 || arg4 || arg5) > + return -EINVAL; > error = MPX_ENABLE_MANAGEMENT(me); > break; > case PR_MPX_DISABLE_MANAGEMENT: > + if (arg2 || arg3 || arg4 || arg5) > + return -EINVAL; > error = MPX_DISABLE_MANAGEMENT(me); > break; > default: > > And this one too: > > commit 3e91ec89f527b9870fe42dcbdb74fd389d123a95 > Author: Catalin Marinas <catalin.marinas@xxxxxxx> > Date: Thu Aug 15 16:44:00 2019 +0100 > > arm64: Tighten the PR_{SET, GET}_TAGGED_ADDR_CTRL prctl() unused arguments > > Require that arg{3,4,5} of the PR_{SET,GET}_TAGGED_ADDR_CTRL prctl and > arg2 of the PR_GET_TAGGED_ADDR_CTRL prctl() are zero rather than ignored > for future extensions. > > Acked-by: Andrey Konovalov <andreyknvl@xxxxxxxxxx> > Signed-off-by: Catalin Marinas <catalin.marinas@xxxxxxx> > Signed-off-by: Will Deacon <will@xxxxxxxxxx> > > diff --git a/kernel/sys.c b/kernel/sys.c > index c6c4d5358bd3..ec48396b4943 100644 > --- a/kernel/sys.c > +++ b/kernel/sys.c > @@ -2499,9 +2499,13 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3, > error = PAC_RESET_KEYS(me, arg2); > break; > case PR_SET_TAGGED_ADDR_CTRL: > + if (arg3 || arg4 || arg5) > + return -EINVAL; > error = SET_TAGGED_ADDR_CTRL(arg2); > break; > case PR_GET_TAGGED_ADDR_CTRL: > + if (arg2 || arg3 || arg4 || arg5) > + return -EINVAL; > error = GET_TAGGED_ADDR_CTRL(); > break; > default: > > In the few calls that util-linux makes without specifying all 5 args, > the kernel seems to not do the checks (in some old prctl(2)s they didn't > have that check, and nobody seems to have cared enough to add it), so > it's more like we're lucky (or unlucky, depending on how you see it). > > > glibc itself doesn't even specify all five arguments in its own calls to > > prctl(). > > glibc itself is wrong. I'm even surprised that the PR_* macros from the > kernel UAPI for arg2 work without specifying the L suffix on them, but > it's probably just luck. > > <https://lore.kernel.org/linux-api/20240528114750.106187-1-alx@xxxxxxxxxx/T/#u> > > > If all five arguments are really required then prctl() wouldn't need to > > be variadic. > > Indeed. I guess that's for historic reasons, rather than actual > necessity; but I don't know for sure. > > > How is random non-zero data less valid than a essentially random zero? > > And if the kernel actually validates this, how has it ever worked before? > > They only added validation for (all) new prctl(2) calls, plus maybe some > old ones, but not all. In the ones used in util-linux that don't > specify zero, I've checked now that the kernel doesn't validate. > > However, a call such as > > prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) > > (this call exists in util-linux) > actually means > > prctl(PR_SET_NO_NEW_PRIVS, 1L, 0L, random, random) > > and it supposedly has been working so far. Those random bits are > probably 0 most of the time, for some reason. And the kernel does check > this one: > > $ sed -n /PR_SET_NO_NEW_PRIVS/,+2p <kernel/sys.c > case PR_SET_NO_NEW_PRIVS: > if (arg2 != 1 || arg3 || arg4 || arg5) > return -EINVAL; > > > Other popular software like systemd or opendjk also don't specify unused arguments. > > I've also checked that the ones that systemd uses without specifying all > 5 args, they are not checked by the kernel. > > > So it doesn't really seem "broken". > > If the patch is more about "being on the safe side", then this should be > > spelled out. > > Still, libc reads those values (on x32) which results in Undefined > Behavior inside glibc. Which is a bad thing. Not broken, because the > compiler has little information to exploit that UB, but not a good thing > either. > > $ grepc __prctl . > ./include/sys/prctl.h:extern int __prctl (int __option, ...); > ./sysdeps/unix/sysv/linux/x86_64/x32/prctl.c:int > __prctl (int option, ...) > { > va_list arg; > va_start (arg, option); > unsigned long int arg2 = va_arg (arg, unsigned long int); > unsigned long int arg3 = va_arg (arg, unsigned long int); > unsigned long int arg4 = va_arg (arg, unsigned long int); > unsigned long int arg5 = va_arg (arg, unsigned long int); > va_end (arg); > return INLINE_SYSCALL_CALL (prctl, option, arg2, arg3, arg4, arg5); > } Or one could say this is not a user problem, and just an implementation detail of libc. For those calls that the kernel ignores the argument, omitting the argument sounds reasonable as a user. I guess the kernel won't expand those APIs, since that would be dangerous (for this precise reason). > > It's arguably less broken than the missing 'L', though. > > > (Plus the cases where documented, required arguments are missing) > > None of the cases where we omit the arguments are checked by the kernel. -- <https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
