Re: [PATCH v1] Call prctl(2) with long integers, specify 5 arguments, and avoid casts

Alejandro Colomar <[email protected]> Sat, 1 Jun 2024 21:32:13 +0200
Newsgroups	org.kernel.vger.util-linux
Message-ID	<3m7c3aemzupjkfbrcijcxm6zroizsyi4sil3mgtcq2sswbmv7y@zjebioivhzfz>
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 <[email protected]>
> 	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 <[email protected]>
> 	    Suggested-by: Michael Kerrisk <[email protected]>
> 	    Signed-off-by: Dave Hansen <[email protected]>
> 	    Cc: Dave Hansen <[email protected]>
> 	    Link: http://lkml.kernel.org/r/[email protected]
> 	    Signed-off-by: Thomas Gleixner <[email protected]>
> 
> 	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 <[email protected]>
> 	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 <[email protected]>
> 	    Signed-off-by: Catalin Marinas <[email protected]>
> 	    Signed-off-by: Will Deacon <[email protected]>
> 
> 	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/[email protected]/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/>
signature.asc (application/pgp-signature, 833 B)