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); } 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. Have a lovely day! Alex -- <https://www.alejandro-colomar.es/>
Attachment:
signature.asc
Description: PGP signature
