[PATCH v2 1/3] syscall_user_dispatch: Allow allowed range wrap-around

[PATCH v2 1/3] syscall_user_dispatch: Allow allowed range wrap-around

[Dmitry Vyukov]

There are two possible scenarios for syscall filtering:
 - having a trusted/allowed range of PCs, and intercepting everything else
 - or the opposite: a single untrusted/intercepted range and allowing
   everything else
The current implementation only allows the former use case due to
allowed range wrap-around check. Allow the latter use case as well
by removing the wrap-around check.
The latter use case is relevant for any kind of sandboxing scenario,
or monitoring behavior of a single library. If a program wants to
intercept syscalls for PC range [START, END) then it needs to call:
prctl(..., END, -(END-START), ...);
which sets a wrap-around range that excludes everything
besides [START, END).

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Gregory Price <gregory.price@memverge.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-kernel@vger.kernel.org
---
 kernel/entry/syscall_user_dispatch.c | 9 +++------
 kernel/sys.c                         | 6 ++++++
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/kernel/entry/syscall_user_dispatch.c b/kernel/entry/syscall_user_dispatch.c
index 5340c5aa89e7d..a0659f0515404 100644
--- a/kernel/entry/syscall_user_dispatch.c
+++ b/kernel/entry/syscall_user_dispatch.c
@@ -37,6 +37,7 @@ bool syscall_user_dispatch(struct pt_regs *regs)
 	struct syscall_user_dispatch *sd = &current->syscall_dispatch;
 	char state;
 
+	/* Note: this check form allows for range wrap-around. */
 	if (likely(instruction_pointer(regs) - sd->offset < sd->len))
 		return false;
 
@@ -80,13 +81,9 @@ static int task_set_syscall_user_dispatch(struct task_struct *task, unsigned lon
 		break;
 	case PR_SYS_DISPATCH_ON:
 		/*
-		 * Validate the direct dispatcher region just for basic
-		 * sanity against overflow and a 0-sized dispatcher
-		 * region.  If the user is able to submit a syscall from
-		 * an address, that address is obviously valid.
+		 * Note: we don't check and allow arbitrary values for
+		 * offset/len in particular to allow range wrap-around.
 		 */
-		if (offset && offset + len <= offset)
-			return -EINVAL;
 
 		/*
 		 * access_ok() will clear memory tags for tagged addresses
diff --git a/kernel/sys.c b/kernel/sys.c
index cb366ff8703af..666322026ad72 100644
--- a/kernel/sys.c
+++ b/kernel/sys.c
@@ -2735,6 +2735,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
 		error = (current->flags & PR_IO_FLUSHER) == PR_IO_FLUSHER;
 		break;
 	case PR_SET_SYSCALL_USER_DISPATCH:
+		/*
+		 * Sign-extend len for 32-bit processes to allow region
+		 * wrap-around.
+		 */
+		if (in_compat_syscall())
+			arg4 = (long)(s32)arg4;
 		error = set_syscall_user_dispatch(arg2, arg3, arg4,
 						  (char __user *) arg5);
 		break;
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH v2 1/3] syscall_user_dispatch: Allow allowed range wrap-around

[Dmitry Vyukov]

On Mon, 24 Feb 2025 at 09:45, Dmitry Vyukov <dvyukov@google.com> wrote:
>
> There are two possible scenarios for syscall filtering:
>  - having a trusted/allowed range of PCs, and intercepting everything else
>  - or the opposite: a single untrusted/intercepted range and allowing
>    everything else
> The current implementation only allows the former use case due to
> allowed range wrap-around check. Allow the latter use case as well
> by removing the wrap-around check.
> The latter use case is relevant for any kind of sandboxing scenario,
> or monitoring behavior of a single library. If a program wants to
> intercept syscalls for PC range [START, END) then it needs to call:
> prctl(..., END, -(END-START), ...);
> which sets a wrap-around range that excludes everything
> besides [START, END).
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> Cc: Gabriel Krisman Bertazi <krisman@collabora.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Andy Lutomirski <luto@kernel.org>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Gregory Price <gregory.price@memverge.com>
> Cc: Marco Elver <elver@google.com>
> Cc: linux-kernel@vger.kernel.org

Any remaining concerns with this series?

Are syscall_user_dispatch patches pulled via x86 tree?

> ---
>  kernel/entry/syscall_user_dispatch.c | 9 +++------
>  kernel/sys.c                         | 6 ++++++
>  2 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/entry/syscall_user_dispatch.c b/kernel/entry/syscall_user_dispatch.c
> index 5340c5aa89e7d..a0659f0515404 100644
> --- a/kernel/entry/syscall_user_dispatch.c
> +++ b/kernel/entry/syscall_user_dispatch.c
> @@ -37,6 +37,7 @@ bool syscall_user_dispatch(struct pt_regs *regs)
>         struct syscall_user_dispatch *sd = &current->syscall_dispatch;
>         char state;
>
> +       /* Note: this check form allows for range wrap-around. */
>         if (likely(instruction_pointer(regs) - sd->offset < sd->len))
>                 return false;
>
> @@ -80,13 +81,9 @@ static int task_set_syscall_user_dispatch(struct task_struct *task, unsigned lon
>                 break;
>         case PR_SYS_DISPATCH_ON:
>                 /*
> -                * Validate the direct dispatcher region just for basic
> -                * sanity against overflow and a 0-sized dispatcher
> -                * region.  If the user is able to submit a syscall from
> -                * an address, that address is obviously valid.
> +                * Note: we don't check and allow arbitrary values for
> +                * offset/len in particular to allow range wrap-around.
>                  */
> -               if (offset && offset + len <= offset)
> -                       return -EINVAL;
>
>                 /*
>                  * access_ok() will clear memory tags for tagged addresses
> diff --git a/kernel/sys.c b/kernel/sys.c
> index cb366ff8703af..666322026ad72 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -2735,6 +2735,12 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
>                 error = (current->flags & PR_IO_FLUSHER) == PR_IO_FLUSHER;
>                 break;
>         case PR_SET_SYSCALL_USER_DISPATCH:
> +               /*
> +                * Sign-extend len for 32-bit processes to allow region
> +                * wrap-around.
> +                */
> +               if (in_compat_syscall())
> +                       arg4 = (long)(s32)arg4;
>                 error = set_syscall_user_dispatch(arg2, arg3, arg4,
>                                                   (char __user *) arg5);
>                 break;
> --
> 2.48.1.601.g30ceb7b040-goog
>

Re: [PATCH v2 1/3] syscall_user_dispatch: Allow allowed range wrap-around

[Thomas Gleixner]

On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
> There are two possible scenarios for syscall filtering:
>  - having a trusted/allowed range of PCs, and intercepting everything else
>  - or the opposite: a single untrusted/intercepted range and allowing
>    everything else
> The current implementation only allows the former use case due to
> allowed range wrap-around check. Allow the latter use case as well
> by removing the wrap-around check.
> The latter use case is relevant for any kind of sandboxing scenario,
> or monitoring behavior of a single library. If a program wants to
> intercept syscalls for PC range [START, END) then it needs to call:
> prctl(..., END, -(END-START), ...);
> which sets a wrap-around range that excludes everything
> besides [START, END).

That's not really intuitive and the implementation changes the prctl()
behaviour in a non backwards compatible way.

Can we please keep the current behaviour and have a new mode. Something
like:

 # define PR_SYS_DISPATCH_OFF            0
 # define PR_SYS_DISPATCH_ON             1
 # define PR_SYS_DISPATCH_EXCLUSIVE_ON   PR_SYS_DISPATCH_ON
 # define PR_SYS_DISPATCH_INCLUSIVE_ON   2

That keeps the current mode backwards compatible and avoids the oddity of

     prctl(..., END, -(END-START), ...);

i.e. this is clearly and obvious distinguishable for user space:

     prctl(..., PR_SYS_DISPATCH_EXCLUSIVE_ON, END, END - START, ...);
     prctl(..., PR_SYS_DISPATCH_INCLUSIVE_ON, END, END - START, ...);

Which makes a lot of sense because these two modes are distinctly
different, no?

PR_SYS_DISPATCH_INCLUSIVE_ON will fail on older kernels and both modes
have a sanity check. PR_SYS_DISPATCH_INCLUSIVE_ON should at least check
for a zero length dispatcher region.

Aside of the better user interface this avoids the in_compat_syscall()
hack. Because then set_syscall_user_dispatch() does the range inversion
and that works completely independent of compat.

> kernel/entry/syscall_user_dispatch.c | 9 +++------
> kernel/sys.c                         | 6 ++++++
> 2 files changed, 9 insertions(+), 6 deletions(-)

This clearly lacks an update of

     Documentation/admin-guide/syscall-user-dispatch.rst

Thanks,

        tglx

Re: [PATCH v2 1/3] syscall_user_dispatch: Allow allowed range wrap-around

[Dmitry Vyukov]

On Sat, 8 Mar 2025 at 12:19, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
> > There are two possible scenarios for syscall filtering:
> >  - having a trusted/allowed range of PCs, and intercepting everything else
> >  - or the opposite: a single untrusted/intercepted range and allowing
> >    everything else
> > The current implementation only allows the former use case due to
> > allowed range wrap-around check. Allow the latter use case as well
> > by removing the wrap-around check.
> > The latter use case is relevant for any kind of sandboxing scenario,
> > or monitoring behavior of a single library. If a program wants to
> > intercept syscalls for PC range [START, END) then it needs to call:
> > prctl(..., END, -(END-START), ...);
> > which sets a wrap-around range that excludes everything
> > besides [START, END).
>
> That's not really intuitive and the implementation changes the prctl()
> behaviour in a non backwards compatible way.
>
> Can we please keep the current behaviour and have a new mode. Something
> like:
>
>  # define PR_SYS_DISPATCH_OFF            0
>  # define PR_SYS_DISPATCH_ON             1
>  # define PR_SYS_DISPATCH_EXCLUSIVE_ON   PR_SYS_DISPATCH_ON
>  # define PR_SYS_DISPATCH_INCLUSIVE_ON   2
>
> That keeps the current mode backwards compatible and avoids the oddity of
>
>      prctl(..., END, -(END-START), ...);
>
> i.e. this is clearly and obvious distinguishable for user space:
>
>      prctl(..., PR_SYS_DISPATCH_EXCLUSIVE_ON, END, END - START, ...);
>      prctl(..., PR_SYS_DISPATCH_INCLUSIVE_ON, END, END - START, ...);
>
> Which makes a lot of sense because these two modes are distinctly
> different, no?
>
> PR_SYS_DISPATCH_INCLUSIVE_ON will fail on older kernels and both modes
> have a sanity check. PR_SYS_DISPATCH_INCLUSIVE_ON should at least check
> for a zero length dispatcher region.
>
> Aside of the better user interface this avoids the in_compat_syscall()
> hack. Because then set_syscall_user_dispatch() does the range inversion
> and that works completely independent of compat.
>
> > kernel/entry/syscall_user_dispatch.c | 9 +++------
> > kernel/sys.c                         | 6 ++++++
> > 2 files changed, 9 insertions(+), 6 deletions(-)
>
> This clearly lacks an update of
>
>      Documentation/admin-guide/syscall-user-dispatch.rst

I like this!

I've just sent v3 with this interface.

[PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Dmitry Vyukov]

Successful syscalls don't change errno, so checking errno is wrong
to ensure that a syscall has failed. For example for the following
sequence:

	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0);
	EXPECT_EQ(EINVAL, errno);
	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, &sel);
	EXPECT_EQ(EINVAL, errno);

only the first syscall may fail and set errno, but the second may succeed
and keep errno intact, and the check will falsely pass.
Or if errno happened to be EINVAL before, even the first check may falsely
pass.

Also use EXPECT/ASSERT consistently. Currently there is an inconsistent mix
without obvious reasons for usage of one or another.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Gregory Price <gregory.price@memverge.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-kernel@vger.kernel.org
---
 .../syscall_user_dispatch/sud_test.c          | 30 +++++++++----------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
index d975a67673299..b0969925ec64c 100644
--- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
+++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
@@ -86,55 +86,53 @@ TEST(bad_prctl_param)
 
 	/* Invalid op */
 	op = -1;
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
-	ASSERT_EQ(EINVAL, errno);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel));
+	EXPECT_EQ(EINVAL, errno);
 
 	/* PR_SYS_DISPATCH_OFF */
 	op = PR_SYS_DISPATCH_OFF;
 
 	/* offset != 0 */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, 0);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, 0));
 	EXPECT_EQ(EINVAL, errno);
 
 	/* len != 0 */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0));
 	EXPECT_EQ(EINVAL, errno);
 
 	/* sel != NULL */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, &sel);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, &sel));
 	EXPECT_EQ(EINVAL, errno);
 
 	/* Valid parameter */
-	errno = 0;
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, 0x0);
-	EXPECT_EQ(0, errno);
+	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x0, 0x0));
 
 	/* PR_SYS_DISPATCH_ON */
 	op = PR_SYS_DISPATCH_ON;
 
 	/* Dispatcher region is bad (offset > 0 && len == 0) */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, &sel);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, &sel));
 	EXPECT_EQ(EINVAL, errno);
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, -1L, 0x0, &sel);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, -1L, 0x0, &sel));
 	EXPECT_EQ(EINVAL, errno);
 
 	/* Invalid selector */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x1, (void *) -1);
-	ASSERT_EQ(EFAULT, errno);
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x1, (void *) -1));
+	EXPECT_EQ(EFAULT, errno);
 
 	/*
 	 * Dispatcher range overflows unsigned long
 	 */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 1, -1L, &sel);
-	ASSERT_EQ(EINVAL, errno) {
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 1, -1L, &sel));
+	EXPECT_EQ(EINVAL, errno) {
 		TH_LOG("Should reject bad syscall range");
 	}
 
 	/*
 	 * Allowed range overflows usigned long
 	 */
-	prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, -1L, 0x1, &sel);
-	ASSERT_EQ(EINVAL, errno) {
+	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, -1L, 0x1, &sel));
+	EXPECT_EQ(EINVAL, errno) {
 		TH_LOG("Should reject bad syscall range");
 	}
 }
-- 
2.48.1.601.g30ceb7b040-goog

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Gregory Price]

On Mon, Feb 24, 2025 at 09:45:26AM +0100, Dmitry Vyukov wrote:
>  	/* len != 0 */
> -	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0);
> +	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0));
>  	EXPECT_EQ(EINVAL, errno);

This patch should probably just be pulled ahead of everything else,
since you change the behavior of the syscall, and now you're updating
the test - but it will fail (since this no longer produces EINVAL).

This patch should probably just be entirely separate, maybe even in
stable?

~Gregory

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Dmitry Vyukov]

On Mon, 3 Mar 2025 at 17:06, Gregory Price <gourry@gourry.net> wrote:
>
> On Mon, Feb 24, 2025 at 09:45:26AM +0100, Dmitry Vyukov wrote:
> >       /* len != 0 */
> > -     prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0);
> > +     EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0xff, 0));
> >       EXPECT_EQ(EINVAL, errno);
>
> This patch should probably just be pulled ahead of everything else,
> since you change the behavior of the syscall, and now you're updating
> the test - but it will fail (since this no longer produces EINVAL).
>
> This patch should probably just be entirely separate, maybe even in
> stable?

Should I do something for this (what)? Or maintainers can pull it?
What tree should pull it?

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Thomas Gleixner]

On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
>
> Also use EXPECT/ASSERT consistently. Currently there is an inconsistent mix
> without obvious reasons for usage of one or another.
>
> Signed-off-by: Dmitry Vyukov <dvyukov@google.com>

As Gregory said, this should be the first patch in the series with a
proper Fixes tag.

>  	/* Invalid op */
>  	op = -1;
> -	prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
> -	ASSERT_EQ(EINVAL, errno);
> +	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel));
> +	EXPECT_EQ(EINVAL, errno);

Seriously?

Something like:

static void prctl_invalid(unsigned long op, unsigned long offs, unsigned long len,
			  void *sel, int err)
{
	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
	EXPECT_EQ(err, errno);
}

static void prctl_valid(unsigned long op, unsigned long offs, unsigned long len,
			void *sel)
{
	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
}

....
	/* Invalid op */
	prctl_invalid(-1, 0, 0, &sel, -EINVAL);
	/* offset != 0 */
	prctl_invalid(PR_SYS_DISPATCH_OFF, 1, 0, NULL, -EINVAL);
        ....
	/* The odd valid test in bad_prctl_param() */
	prctl_valid(PR_SYS_DISPATCH_OFF, 0, 0, NULL);

But that's not enough macro uglyness sprinkled all over the place and
too readable, right?

Thanks,

        tglx

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Dmitry Vyukov]

On Sat, 8 Mar 2025 at 13:34, Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
> >
> > Also use EXPECT/ASSERT consistently. Currently there is an inconsistent mix
> > without obvious reasons for usage of one or another.
> >
> > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
>
> As Gregory said, this should be the first patch in the series with a
> proper Fixes tag.
>
> >       /* Invalid op */
> >       op = -1;
> > -     prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
> > -     ASSERT_EQ(EINVAL, errno);

> > +     EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel));
> > +     EXPECT_EQ(EINVAL, errno);
>
> Seriously?
>
> Something like:
>
> static void prctl_invalid(unsigned long op, unsigned long offs, unsigned long len,
>                           void *sel, int err)
> {
>         EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
>         EXPECT_EQ(err, errno);
> }
>
> static void prctl_valid(unsigned long op, unsigned long offs, unsigned long len,
>                         void *sel)
> {
>         EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
> }
>
> ....
>         /* Invalid op */
>         prctl_invalid(-1, 0, 0, &sel, -EINVAL);
>         /* offset != 0 */
>         prctl_invalid(PR_SYS_DISPATCH_OFF, 1, 0, NULL, -EINVAL);
>         ....
>         /* The odd valid test in bad_prctl_param() */
>         prctl_valid(PR_SYS_DISPATCH_OFF, 0, 0, NULL);
>
> But that's not enough macro uglyness sprinkled all over the place and
> too readable, right?

The EXPECT* macros unfortunately can't be used in helper functions,
they require some hidden _metadata variable that is present only in
TEST/TEST_F functions:

sud_test.c: In function ‘prctl_valid’:
../kselftest_harness.h:107:45: error: ‘_metadata’ undeclared (first
use in this function)
  107 |                         __FILE__, __LINE__, _metadata->name,
##__VA_ARGS__)

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Thomas Weißschuh]

On 2025-05-21 12:07:13+0200, Dmitry Vyukov wrote:
> On Sat, 8 Mar 2025 at 13:34, Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
> > >
> > > Also use EXPECT/ASSERT consistently. Currently there is an inconsistent mix
> > > without obvious reasons for usage of one or another.
> > >
> > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> >
> > As Gregory said, this should be the first patch in the series with a
> > proper Fixes tag.
> >
> > >       /* Invalid op */
> > >       op = -1;
> > > -     prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
> > > -     ASSERT_EQ(EINVAL, errno);
> 
> > > +     EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel));
> > > +     EXPECT_EQ(EINVAL, errno);
> >
> > Seriously?
> >
> > Something like:
> >
> > static void prctl_invalid(unsigned long op, unsigned long offs, unsigned long len,
> >                           void *sel, int err)
> > {
> >         EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
> >         EXPECT_EQ(err, errno);
> > }
> >
> > static void prctl_valid(unsigned long op, unsigned long offs, unsigned long len,
> >                         void *sel)
> > {
> >         EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
> > }
> >
> > ....
> >         /* Invalid op */
> >         prctl_invalid(-1, 0, 0, &sel, -EINVAL);
> >         /* offset != 0 */
> >         prctl_invalid(PR_SYS_DISPATCH_OFF, 1, 0, NULL, -EINVAL);
> >         ....
> >         /* The odd valid test in bad_prctl_param() */
> >         prctl_valid(PR_SYS_DISPATCH_OFF, 0, 0, NULL);
> >
> > But that's not enough macro uglyness sprinkled all over the place and
> > too readable, right?
> 
> The EXPECT* macros unfortunately can't be used in helper functions,
> they require some hidden _metadata variable that is present only in
> TEST/TEST_F functions:
> 
> sud_test.c: In function ‘prctl_valid’:
> ../kselftest_harness.h:107:45: error: ‘_metadata’ undeclared (first
> use in this function)
>   107 |                         __FILE__, __LINE__, _metadata->name,
> ##__VA_ARGS__)

You can pass the _metadata parameter to your helper functions.
While it's a bit iffy, many selftests do this.
Also the upcoming harness selftests will make sure that this pattern
keeps working.


Thomas

Re: [PATCH v2 2/3] selftests: Fix errno checking in syscall_user_dispatch test

[Dmitry Vyukov]

On Wed, 21 May 2025 at 13:10, Thomas Weißschuh <linux@weissschuh.net> wrote:
>
> On 2025-05-21 12:07:13+0200, Dmitry Vyukov wrote:
> > On Sat, 8 Mar 2025 at 13:34, Thomas Gleixner <tglx@linutronix.de> wrote:
> > >
> > > On Mon, Feb 24 2025 at 09:45, Dmitry Vyukov wrote:
> > > >
> > > > Also use EXPECT/ASSERT consistently. Currently there is an inconsistent mix
> > > > without obvious reasons for usage of one or another.
> > > >
> > > > Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
> > >
> > > As Gregory said, this should be the first patch in the series with a
> > > proper Fixes tag.
> > >
> > > >       /* Invalid op */
> > > >       op = -1;
> > > > -     prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel);
> > > > -     ASSERT_EQ(EINVAL, errno);
> >
> > > > +     EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0, 0, &sel));
> > > > +     EXPECT_EQ(EINVAL, errno);
> > >
> > > Seriously?
> > >
> > > Something like:
> > >
> > > static void prctl_invalid(unsigned long op, unsigned long offs, unsigned long len,
> > >                           void *sel, int err)
> > > {
> > >         EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
> > >         EXPECT_EQ(err, errno);
> > > }
> > >
> > > static void prctl_valid(unsigned long op, unsigned long offs, unsigned long len,
> > >                         void *sel)
> > > {
> > >         EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, offs, len, 0, (unsigned long)sel));
> > > }
> > >
> > > ....
> > >         /* Invalid op */
> > >         prctl_invalid(-1, 0, 0, &sel, -EINVAL);
> > >         /* offset != 0 */
> > >         prctl_invalid(PR_SYS_DISPATCH_OFF, 1, 0, NULL, -EINVAL);
> > >         ....
> > >         /* The odd valid test in bad_prctl_param() */
> > >         prctl_valid(PR_SYS_DISPATCH_OFF, 0, 0, NULL);
> > >
> > > But that's not enough macro uglyness sprinkled all over the place and
> > > too readable, right?
> >
> > The EXPECT* macros unfortunately can't be used in helper functions,
> > they require some hidden _metadata variable that is present only in
> > TEST/TEST_F functions:
> >
> > sud_test.c: In function ‘prctl_valid’:
> > ../kselftest_harness.h:107:45: error: ‘_metadata’ undeclared (first
> > use in this function)
> >   107 |                         __FILE__, __LINE__, _metadata->name,
> > ##__VA_ARGS__)
>
> You can pass the _metadata parameter to your helper functions.
> While it's a bit iffy, many selftests do this.
> Also the upcoming harness selftests will make sure that this pattern
> keeps working.

This works, thanks. I will include this in v3.

[PATCH v2 3/3] selftests: Extend syscall_user_dispatch test to check allowed range

[Dmitry Vyukov]

Add a test that ensures that PR_SET_SYSCALL_USER_DISPATCH respects
the specified allowed PC range. The test includes both a continuous
range and a wrap-around range.

Signed-off-by: Dmitry Vyukov <dvyukov@google.com>
Cc: Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: Gregory Price <gregory.price@memverge.com>
Cc: Marco Elver <elver@google.com>
Cc: linux-kernel@vger.kernel.org

---
Changes in v2:
 - add tests for 0-sized range
 - change range setup in the test to be fatal
---
 .../syscall_user_dispatch/sud_test.c          | 85 ++++++++++++-------
 1 file changed, 54 insertions(+), 31 deletions(-)

diff --git a/tools/testing/selftests/syscall_user_dispatch/sud_test.c b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
index b0969925ec64c..302089ccd103c 100644
--- a/tools/testing/selftests/syscall_user_dispatch/sud_test.c
+++ b/tools/testing/selftests/syscall_user_dispatch/sud_test.c
@@ -10,6 +10,8 @@
 #include <sys/sysinfo.h>
 #include <sys/syscall.h>
 #include <signal.h>
+#include <stdbool.h>
+#include <stdlib.h>
 
 #include <asm/unistd.h>
 #include "../kselftest_harness.h"
@@ -110,31 +112,15 @@ TEST(bad_prctl_param)
 	/* PR_SYS_DISPATCH_ON */
 	op = PR_SYS_DISPATCH_ON;
 
-	/* Dispatcher region is bad (offset > 0 && len == 0) */
-	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, &sel));
-	EXPECT_EQ(EINVAL, errno);
-	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, -1L, 0x0, &sel));
-	EXPECT_EQ(EINVAL, errno);
+	/* All ranges are allowed */
+	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x1, 0x0, &sel));
+	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, -1L, 0x0, &sel));
+	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 1, -1L, &sel));
+	EXPECT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, -1L, 0x1, &sel));
 
 	/* Invalid selector */
 	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, op, 0x0, 0x1, (void *) -1));
 	EXPECT_EQ(EFAULT, errno);
-
-	/*
-	 * Dispatcher range overflows unsigned long
-	 */
-	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 1, -1L, &sel));
-	EXPECT_EQ(EINVAL, errno) {
-		TH_LOG("Should reject bad syscall range");
-	}
-
-	/*
-	 * Allowed range overflows usigned long
-	 */
-	EXPECT_EQ(-1, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, -1L, 0x1, &sel));
-	EXPECT_EQ(EINVAL, errno) {
-		TH_LOG("Should reject bad syscall range");
-	}
 }
 
 /*
@@ -145,11 +131,13 @@ char glob_sel;
 int nr_syscalls_emulated;
 int si_code;
 int si_errno;
+unsigned long syscall_addr;
 
 static void handle_sigsys(int sig, siginfo_t *info, void *ucontext)
 {
 	si_code = info->si_code;
 	si_errno = info->si_errno;
+	syscall_addr = (unsigned long)info->si_call_addr;
 
 	if (info->si_syscall == MAGIC_SYSCALL_1)
 		nr_syscalls_emulated++;
@@ -172,26 +160,29 @@ static void handle_sigsys(int sig, siginfo_t *info, void *ucontext)
 #endif
 }
 
-TEST(dispatch_and_return)
+int setup_sigsys_handler(void)
 {
-	long ret;
 	struct sigaction act;
 	sigset_t mask;
 
-	glob_sel = 0;
-	nr_syscalls_emulated = 0;
-	si_code = 0;
-	si_errno = 0;
-
 	memset(&act, 0, sizeof(act));
 	sigemptyset(&mask);
-
 	act.sa_sigaction = handle_sigsys;
 	act.sa_flags = SA_SIGINFO;
 	act.sa_mask = mask;
+	return sigaction(SIGSYS, &act, NULL);
+}
 
-	ret = sigaction(SIGSYS, &act, NULL);
-	ASSERT_EQ(0, ret);
+TEST(dispatch_and_return)
+{
+	long ret;
+
+	glob_sel = 0;
+	nr_syscalls_emulated = 0;
+	si_code = 0;
+	si_errno = 0;
+
+	ASSERT_EQ(0, setup_sigsys_handler());
 
 	/* Make sure selector is good prior to prctl. */
 	SYSCALL_DISPATCH_OFF(glob_sel);
@@ -321,4 +312,36 @@ TEST(direct_dispatch_range)
 	}
 }
 
+bool test_range(unsigned long offset, unsigned long length)
+{
+	nr_syscalls_emulated = 0;
+	SYSCALL_DISPATCH_OFF(glob_sel);
+	if (prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, offset, length, &glob_sel))
+		abort();
+	SYSCALL_DISPATCH_ON(glob_sel);
+	return syscall(MAGIC_SYSCALL_1) == MAGIC_SYSCALL_1 && nr_syscalls_emulated == 1;
+}
+
+TEST(dispatch_range)
+{
+	ASSERT_EQ(0, setup_sigsys_handler());
+	ASSERT_EQ(0, prctl(PR_SET_SYSCALL_USER_DISPATCH, PR_SYS_DISPATCH_ON, 0, 0, &glob_sel));
+	SYSCALL_DISPATCH_ON(glob_sel);
+	ASSERT_EQ(MAGIC_SYSCALL_1, syscall(MAGIC_SYSCALL_1));
+	TH_LOG("syscall_addr=0x%lx", syscall_addr);
+	EXPECT_FALSE(test_range(syscall_addr, 1));
+	EXPECT_FALSE(test_range(syscall_addr-100, 200));
+	EXPECT_TRUE(test_range(syscall_addr+1, 100));
+	EXPECT_TRUE(test_range(syscall_addr-100, 100));
+	/* Wrap-around tests for everything except for a single PC. */
+	EXPECT_TRUE(test_range(syscall_addr+1, -1));
+	EXPECT_FALSE(test_range(syscall_addr, -1));
+	EXPECT_FALSE(test_range(syscall_addr+2, -1));
+	/* 0-size range does not match anything. */
+	EXPECT_TRUE(test_range(syscall_addr-1, 0));
+	EXPECT_TRUE(test_range(syscall_addr, 0));
+	EXPECT_TRUE(test_range(syscall_addr+1, 0));
+	SYSCALL_DISPATCH_OFF(glob_sel);
+}
+
 TEST_HARNESS_MAIN
-- 
2.48.1.601.g30ceb7b040-goog