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

On 01/06/24 09:23, 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);
> 	}
> 
> It's arguably less broken than the missing 'L', though.

The x32 and or1k (which also uses similar implementation) does seems broken
without checking the 'option' argument to see which arg we can va_arg.

The problem is adding this logic on libc will add some forward-compatibility
that we try to avoid (newer kernel prctl additions might now work correctly).

I am not sure why we haven't switch x32 back to the assembly wrappers
with 6a04404521ac4119ae36827eeb288ea84eee7cf6 fix (BZ#29770).  H.J, can
use remove the x32 C version (and also or1k as well)?

> 
>> (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
>