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

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

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 11494 bytes --]

Since libc's prctl(2) wrapper is a variadic function, arguments must
have the right width.  Otherwise, the behavior is undefined.

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.

Also, avoid some casts to unsigned long, by changing the type of the
parameter to some local wrappers.

And use consistently 0L.  0UL is basically the same, and uses one more
character.  Keep it short.

Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=6698b096a6f5342cb9b338c237ed875a8635497a>
Link: <https://lore.kernel.org/linux-man/ddbdyaiptesjalgfmztxideej67e3yaob7ucsmbf6qvriwxiif@dohhxrqgwhrf/T/#med306b5b003f9cc7cc2de69fcdd7ee2d056d0954>
Cc: Xi Ruoyao <xry111@xry111.site>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
Range-diff against v0:
-:  --------- > 1:  afd73139e Call prctl(2) with long integers, specify 5 arguments, and avoid casts

 include/seccomp.h            |  2 +-
 lib/caputils.c               |  4 ++--
 lib/env.c                    |  4 ++--
 misc-utils/enosys.c          |  4 ++--
 schedutils/coresched.c       |  6 +++---
 sys-utils/setpriv-landlock.c |  2 +-
 sys-utils/setpriv.c          | 34 ++++++++++++++++------------------
 tests/helpers/test_mkfds.c   |  2 +-
 8 files changed, 28 insertions(+), 30 deletions(-)

diff --git a/include/seccomp.h b/include/seccomp.h
index 2b211439e..8c65b5e8c 100644
--- a/include/seccomp.h
+++ b/include/seccomp.h
@@ -18,7 +18,7 @@ static int ul_set_seccomp_filter_spec_allow(const struct sock_fprog *prog)
 		return 0;
 #endif
 
-	return prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, prog);
+	return prctl(PR_SET_SECCOMP, SECCOMP_MODE_FILTER, prog, 0L, 0L);
 }
 
 #endif /* UL_SECCOMP_H */
diff --git a/lib/caputils.c b/lib/caputils.c
index 23866c071..fd7037ff9 100644
--- a/lib/caputils.c
+++ b/lib/caputils.c
@@ -29,7 +29,7 @@
 static int test_cap(unsigned int cap)
 {
 	/* prctl returns 0 or 1 for valid caps, -1 otherwise */
-	return prctl(PR_CAPBSET_READ, cap, 0, 0, 0) >= 0;
+	return prctl(PR_CAPBSET_READ, cap, 0L, 0L, 0L) >= 0;
 }
 
 static int cap_last_by_bsearch(int *ret)
@@ -120,7 +120,7 @@ void cap_permitted_to_ambient(void)
 			continue;
 
 		if ((effective & (1ULL << cap))
-		    && prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, cap, 0, 0) < 0)
+		    && prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE, cap, 0L, 0L) < 0)
 			err(EXIT_FAILURE, _("prctl(PR_CAP_AMBIENT) failed"));
 	}
 }
diff --git a/lib/env.c b/lib/env.c
index 2bdfe5697..5d9ab2456 100644
--- a/lib/env.c
+++ b/lib/env.c
@@ -191,11 +191,11 @@ char *safe_getenv(const char *arg)
 	if ((getuid() != geteuid()) || (getgid() != getegid()))
 		return NULL;
 #ifdef HAVE_PRCTL
-	if (prctl(PR_GET_DUMPABLE, 0, 0, 0, 0) == 0)
+	if (prctl(PR_GET_DUMPABLE, 0L, 0L, 0L, 0L) == 0)
 		return NULL;
 #else
 #if (defined(linux) && defined(SYS_prctl))
-	if (syscall(SYS_prctl, PR_GET_DUMPABLE, 0, 0, 0, 0) == 0)
+	if (syscall(SYS_prctl, PR_GET_DUMPABLE, 0L, 0L, 0L, 0L) == 0)
 		return NULL;
 #endif
 #endif
diff --git a/misc-utils/enosys.c b/misc-utils/enosys.c
index 1410676dd..acf4428b6 100644
--- a/misc-utils/enosys.c
+++ b/misc-utils/enosys.c
@@ -290,10 +290,10 @@ int main(int argc, char **argv)
 	/* *SET* below will return EINVAL when either the filter is invalid or
 	 * seccomp is not supported. To distinguish those cases do a *GET* here
 	 */
-	if (prctl(PR_GET_SECCOMP) == -1 && errno == EINVAL)
+	if (prctl(PR_GET_SECCOMP, 0L, 0L, 0L, 0L) == -1 && errno == EINVAL)
 		err(EXIT_NOTSUPP, _("Seccomp non-functional"));
 
-	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0))
+	if (prctl(PR_SET_NO_NEW_PRIVS, 1L, 0L, 0L, 0L))
 		err_nosys(EXIT_FAILURE, _("Could not run prctl(PR_SET_NO_NEW_PRIVS)"));
 
 	if (ul_set_seccomp_filter_spec_allow(&prog))
diff --git a/schedutils/coresched.c b/schedutils/coresched.c
index 9d8be3e12..7844f98be 100644
--- a/schedutils/coresched.c
+++ b/schedutils/coresched.c
@@ -129,20 +129,20 @@ static sched_core_cookie core_sched_get_cookie(pid_t pid)
 
 static void core_sched_create_cookie(pid_t pid, sched_core_scope type)
 {
-	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE, pid, type, 0))
+	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_CREATE, pid, type, 0L))
 		err(EXIT_FAILURE, _("Failed to create cookie for PID %d"), pid);
 }
 
 static void core_sched_pull_cookie(pid_t from)
 {
 	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_FROM, from,
-		  PR_SCHED_CORE_SCOPE_THREAD, 0))
+		  PR_SCHED_CORE_SCOPE_THREAD, 0L))
 		err(EXIT_FAILURE, _("Failed to pull cookie from PID %d"), from);
 }
 
 static void core_sched_push_cookie(pid_t to, sched_core_scope type)
 {
-	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, to, type, 0))
+	if (prctl(PR_SCHED_CORE, PR_SCHED_CORE_SHARE_TO, to, type, 0L))
 		err(EXIT_FAILURE, _("Failed to push cookie to PID %d"), to);
 }
 
diff --git a/sys-utils/setpriv-landlock.c b/sys-utils/setpriv-landlock.c
index 00ad38c61..18c698325 100644
--- a/sys-utils/setpriv-landlock.c
+++ b/sys-utils/setpriv-landlock.c
@@ -187,7 +187,7 @@ void do_landlock(const struct setpriv_landlock_opts *opts)
 			err(SETPRIV_EXIT_PRIVERR, _("adding landlock rule failed"));
 	}
 
-	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1)
+	if (prctl(PR_SET_NO_NEW_PRIVS, 1L, 0L, 0L, 0L) == -1)
 		err(SETPRIV_EXIT_PRIVERR, _("disallow granting new privileges for landlock failed"));
 
 	if (landlock_restrict_self(fd, 0) == -1)
diff --git a/sys-utils/setpriv.c b/sys-utils/setpriv.c
index 4b0543101..0db09bf74 100644
--- a/sys-utils/setpriv.c
+++ b/sys-utils/setpriv.c
@@ -165,7 +165,7 @@ static void __attribute__((__noreturn__)) usage(void)
 	exit(EXIT_SUCCESS);
 }
 
-static int has_cap(enum cap_type which, unsigned int i)
+static int has_cap(enum cap_type which, unsigned long i)
 {
 	switch (which) {
 	case CAP_TYPE_EFFECTIVE:
@@ -174,8 +174,7 @@ static int has_cap(enum cap_type which, unsigned int i)
 	case CAP_TYPE_PERMITTED:
 		return capng_have_capability((capng_type_t)which, i);
 	case CAP_TYPE_AMBIENT:
-		return prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET,
-				(unsigned long) i, 0UL, 0UL);
+		return prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_IS_SET, i, 0L, 0L);
 	default:
 		warnx(_("invalid capability type"));
 		return -1;
@@ -223,7 +222,7 @@ static void dump_one_secbit(int *first, int *bits, int bit, const char *name)
 static void dump_securebits(void)
 {
 	int first = 1;
-	int bits = prctl(PR_GET_SECUREBITS, 0, 0, 0, 0);
+	int bits = prctl(PR_GET_SECUREBITS, 0L, 0L, 0L, 0L);
 
 	if (bits < 0) {
 		warnx(_("getting process secure bits failed"));
@@ -323,7 +322,7 @@ static void dump_pdeathsig(void)
 {
 	int pdeathsig;
 
-	if (prctl(PR_GET_PDEATHSIG, &pdeathsig) != 0) {
+	if (prctl(PR_GET_PDEATHSIG, &pdeathsig, 0L, 0L, 0L) != 0) {
 		warn(_("get pdeathsig failed"));
 		return;
 	}
@@ -363,7 +362,7 @@ static void dump(int dumplevel)
 
 	dump_groups();
 
-	x = prctl(PR_GET_NO_NEW_PRIVS, 0, 0, 0, 0);
+	x = prctl(PR_GET_NO_NEW_PRIVS, 0L, 0L, 0L, 0L);
 	if (0 <= x)
 		printf("no_new_privs: %d\n", x);
 	else
@@ -449,7 +448,7 @@ static void parse_groups(struct privctx *opts, const char *str)
 static void parse_pdeathsig(struct privctx *opts, const char *str)
 {
 	if (!strcmp(str, "keep")) {
-		if (prctl(PR_GET_PDEATHSIG, &opts->pdeathsig) != 0)
+		if (prctl(PR_GET_PDEATHSIG, &opts->pdeathsig, 0L, 0L, 0L) != 0)
 			errx(SETPRIV_EXIT_PRIVERR,
 				 _("failed to get parent death signal"));
 	} else if (!strcmp(str, "clear")) {
@@ -495,8 +494,7 @@ static void bump_cap(unsigned int cap)
 		capng_update(CAPNG_ADD, CAPNG_EFFECTIVE, cap);
 }
 
-static int cap_update(capng_act_t action,
-		enum cap_type type, unsigned int cap)
+static int cap_update(capng_act_t action, enum cap_type type, unsigned long cap)
 {
 	switch (type) {
 		case CAP_TYPE_EFFECTIVE:
@@ -510,10 +508,10 @@ static int cap_update(capng_act_t action,
 
 			if (action == CAPNG_ADD)
 				ret = prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_RAISE,
-						(unsigned long) cap, 0UL, 0UL);
+						cap, 0L, 0L);
 			else
 				ret = prctl(PR_CAP_AMBIENT, PR_CAP_AMBIENT_LOWER,
-						(unsigned long) cap, 0UL, 0UL);
+						cap, 0L, 0L);
 
 			return ret;
 		}
@@ -565,7 +563,7 @@ static void parse_securebits(struct privctx *opts, const char *arg)
 	char *c;
 
 	opts->have_securebits = 1;
-	opts->securebits = prctl(PR_GET_SECUREBITS, 0, 0, 0, 0);
+	opts->securebits = prctl(PR_GET_SECUREBITS, 0L, 0L, 0L, 0L);
 	if (opts->securebits < 0)
 		err(SETPRIV_EXIT_PRIVERR, _("getting process secure bits failed"));
 
@@ -687,10 +685,10 @@ static void do_seccomp_filter(const char *file)
 	/* *SET* below will return EINVAL when either the filter is invalid or
 	 * seccomp is not supported. To distinguish those cases do a *GET* here
 	 */
-	if (prctl(PR_GET_SECCOMP) == -1 && errno == EINVAL)
+	if (prctl(PR_GET_SECCOMP, 0L, 0L, 0L, 0L) == -1 && errno == EINVAL)
 		err(SETPRIV_EXIT_PRIVERR, _("Seccomp non-functional"));
 
-	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0))
+	if (prctl(PR_SET_NO_NEW_PRIVS, 1L, 0L, 0L, 0L))
 		err(SETPRIV_EXIT_PRIVERR, _("Could not run prctl(PR_SET_NO_NEW_PRIVS)"));
 
 	if (ul_set_seccomp_filter_spec_allow(&prog))
@@ -1059,7 +1057,7 @@ int main(int argc, char **argv)
 		do_reset_environ(pw);
 	}
 
-	if (opts.nnp && prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) == -1)
+	if (opts.nnp && prctl(PR_SET_NO_NEW_PRIVS, 1L, 0L, 0L, 0L) == -1)
 		err(EXIT_FAILURE, _("disallow granting new privileges failed"));
 
 	if (opts.selinux_label)
@@ -1069,7 +1067,7 @@ int main(int argc, char **argv)
 	if (opts.seccomp_filter)
 		do_seccomp_filter(opts.seccomp_filter);
 
-	if (prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0) == -1)
+	if (prctl(PR_SET_KEEPCAPS, 1L, 0L, 0L, 0L) == -1)
 		err(EXIT_FAILURE, _("keep process capabilities failed"));
 
 	/* We're going to want CAP_SETPCAP, CAP_SETUID, and CAP_SETGID if
@@ -1102,7 +1100,7 @@ int main(int argc, char **argv)
 			err(SETPRIV_EXIT_PRIVERR, _("setgroups failed"));
 	}
 
-	if (opts.have_securebits && prctl(PR_SET_SECUREBITS, opts.securebits, 0, 0, 0) != 0)
+	if (opts.have_securebits && prctl(PR_SET_SECUREBITS, opts.securebits, 0L, 0L, 0L) != 0)
 		err(SETPRIV_EXIT_PRIVERR, _("set process securebits failed"));
 
 	if (opts.bounding_set) {
@@ -1123,7 +1121,7 @@ int main(int argc, char **argv)
 	}
 
 	/* Clear or set parent death signal */
-	if (opts.pdeathsig && prctl(PR_SET_PDEATHSIG, opts.pdeathsig < 0 ? 0 : opts.pdeathsig) != 0)
+	if (opts.pdeathsig && prctl(PR_SET_PDEATHSIG, opts.pdeathsig < 0 ? 0L : opts.pdeathsig, 0L, 0L, 0L) != 0)
 		err(SETPRIV_EXIT_PRIVERR, _("set parent death signal failed"));
 
 	do_landlock(&opts.landlock);
diff --git a/tests/helpers/test_mkfds.c b/tests/helpers/test_mkfds.c
index 60ebdd676..cfaa1f2ac 100644
--- a/tests/helpers/test_mkfds.c
+++ b/tests/helpers/test_mkfds.c
@@ -4372,7 +4372,7 @@ static void list_parameters(const char *factory_name)
 
 static void rename_self(const char *comm)
 {
-	if (prctl(PR_SET_NAME, (unsigned long)comm, 0, 0, 0) < 0)
+	if (prctl(PR_SET_NAME, (unsigned long)comm, 0L, 0L, 0L) < 0)
 		err(EXIT_FAILURE, "failed to rename self via prctl: %s", comm);
 }
 
-- 
2.45.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Thomas Weißschuh]

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.
glibc itself doesn't even specify all five arguments in its own calls to
prctl().

If all five arguments are really required then prctl() wouldn't need to
be variadic.

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?

Other popular software like systemd or opendjk also don't specify unused arguments.

So it doesn't really seem "broken".
If the patch is more about "being on the safe side", then this should be
spelled out.
(Plus the cases where documented, required arguments are missing)

> Also, avoid some casts to unsigned long, by changing the type of the
> parameter to some local wrappers.
> 
> And use consistently 0L.  0UL is basically the same, and uses one more
> character.  Keep it short.
> 
> Link: <https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=6698b096a6f5342cb9b338c237ed875a8635497a>
> Link: <https://lore.kernel.org/linux-man/ddbdyaiptesjalgfmztxideej67e3yaob7ucsmbf6qvriwxiif@dohhxrqgwhrf/T/#med306b5b003f9cc7cc2de69fcdd7ee2d056d0954>
> Cc: Xi Ruoyao <xry111@xry111.site>
> Signed-off-by: Alejandro Colomar <alx@kernel.org>
> ---
> Range-diff against v0:
> -:  --------- > 1:  afd73139e Call prctl(2) with long integers, specify 5 arguments, and avoid casts
> 
>  include/seccomp.h            |  2 +-
>  lib/caputils.c               |  4 ++--
>  lib/env.c                    |  4 ++--
>  misc-utils/enosys.c          |  4 ++--
>  schedutils/coresched.c       |  6 +++---
>  sys-utils/setpriv-landlock.c |  2 +-
>  sys-utils/setpriv.c          | 34 ++++++++++++++++------------------
>  tests/helpers/test_mkfds.c   |  2 +-
>  8 files changed, 28 insertions(+), 30 deletions(-)

<snip>

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

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 6972 bytes --]

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@linux.intel.com>
	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@amacapital.net>
	    Suggested-by: Michael Kerrisk <mtk.manpages@gmail.com>
	    Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
	    Cc: Dave Hansen <dave@sr71.net>
	    Link: http://lkml.kernel.org/r/20150108223022.7F56FD13@viggo.jf.intel.com
	    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

	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@arm.com>
	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@google.com>
	    Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
	    Signed-off-by: Will Deacon <will@kernel.org>

	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@kernel.org/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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Alejandro Colomar]

[-- Attachment #1: Type: text/plain, Size: 7673 bytes --]

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@linux.intel.com>
> 	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@amacapital.net>
> 	    Suggested-by: Michael Kerrisk <mtk.manpages@gmail.com>
> 	    Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> 	    Cc: Dave Hansen <dave@sr71.net>
> 	    Link: http://lkml.kernel.org/r/20150108223022.7F56FD13@viggo.jf.intel.com
> 	    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> 	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@arm.com>
> 	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@google.com>
> 	    Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> 	    Signed-off-by: Will Deacon <will@kernel.org>
> 
> 	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@kernel.org/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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Karel Zak]

On Sat, Jun 01, 2024 at 11:31:56AM +0200, Alejandro Colomar wrote:
> Since libc's prctl(2) wrapper is a variadic function, arguments must
> have the right width.  Otherwise, the behavior is undefined.

Created PR to test it by CI:
https://github.com/util-linux/util-linux/pull/3085

    Karel


-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com