[PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Eyal Birger]

uretprobe(2) is an performance enhancement system call added to improve
uretprobes on x86_64.

Confinement environments such as Docker are not aware of this new system
call and kill confined processes when uretprobes are attached to them.

Since uretprobe is a "kernel implementation detail" system call which is
not used by userspace application code directly, pass this system call
through seccomp without forcing existing userspace confinement environments
to be changed.

To: Kees Cook <kees@kernel.org>
To: Andy Lutomirski <luto@amacapital.net>
To: Will Drewry <wad@chromium.org>
To: Oleg Nesterov <oleg@redhat.com>
To: Masami Hiramatsu (Google) <mhiramat@kernel.org>
To: Jiri Olsa <jolsa@kernel.org>
To: Andrii Nakryiko <andrii@kernel.org>
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>

Eyal Birger (2):
  seccomp: passthrough uretprobe systemcall without filtering
  selftests/seccomp: validate uretprobe syscall passes through seccomp

 kernel/seccomp.c                              |  24 ++-
 tools/testing/selftests/seccomp/seccomp_bpf.c | 195 ++++++++++++++++++
 2 files changed, 216 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH v3 1/2] seccomp: passthrough uretprobe systemcall without filtering

[Eyal Birger]

When attaching uretprobes to processes running inside docker, the attached
process is segfaulted when encountering the retprobe.

The reason is that now that uretprobe is a system call the default seccomp
filters in docker block it as they only allow a specific set of known
syscalls. This is true for other userspace applications which use seccomp
to control their syscall surface.

Since uretprobe is a "kernel implementation detail" system call which is
not used by userspace application code directly, it is impractical and
there's very little point in forcing all userspace applications to
explicitly allow it in order to avoid crashing tracked processes.

Pass this systemcall through seccomp without depending on configuration.

Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
uses the same number as __NR_uretprobe so the syscall isn't forced in the
compat bitmap.

Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
Reported-by: Rafael Buchbinder <rafi@rbk.io>
Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
Link: https://lore.kernel.org/lkml/20250121182939.33d05470@gandalf.local.home/T/#me2676c378eff2d6a33f3054fed4a5f3afa64e65b
Link: https://lore.kernel.org/lkml/20250128145806.1849977-1-eyal.birger@gmail.com/
Cc: stable@vger.kernel.org
Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
v3: no change - deferring 32bit compat handling as there aren't plans to
    support this syscall in compat mode.
v2: use action_cache bitmap and mode1 array to check the syscall
---
 kernel/seccomp.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f59381c4a2ff..09b6f8e6db51 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
 
 #ifdef SECCOMP_ARCH_NATIVE
 /**
- * seccomp_is_const_allow - check if filter is constant allow with given data
+ * seccomp_is_filter_const_allow - check if filter is constant allow with given data
  * @fprog: The BPF programs
  * @sd: The seccomp data to check against, only syscall number and arch
  *      number are considered constant.
  */
-static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
-				   struct seccomp_data *sd)
+static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
+					  struct seccomp_data *sd)
 {
 	unsigned int reg_value = 0;
 	unsigned int pc;
@@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
 	return false;
 }
 
+static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
+				   struct seccomp_data *sd)
+{
+#ifdef __NR_uretprobe
+	if (sd->nr == __NR_uretprobe
+#ifdef SECCOMP_ARCH_COMPAT
+	    && sd->arch != SECCOMP_ARCH_COMPAT
+#endif
+	   )
+		return true;
+#endif
+
+	return seccomp_is_filter_const_allow(fprog, sd);
+}
+
 static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
 					 void *bitmap, const void *bitmap_prev,
 					 size_t bitmap_size, int arch)
@@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
  */
 static const int mode1_syscalls[] = {
 	__NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
+#ifdef __NR_uretprobe
+	__NR_uretprobe,
+#endif
 	-1, /* negative terminated */
 };
 
-- 
2.43.0

Re: [PATCH v3 1/2] seccomp: passthrough uretprobe systemcall without filtering

[Kees Cook]

On Sun, Feb 02, 2025 at 08:29:20AM -0800, Eyal Birger wrote:
> When attaching uretprobes to processes running inside docker, the attached
> process is segfaulted when encountering the retprobe.
> 
> The reason is that now that uretprobe is a system call the default seccomp
> filters in docker block it as they only allow a specific set of known
> syscalls. This is true for other userspace applications which use seccomp
> to control their syscall surface.
> 
> Since uretprobe is a "kernel implementation detail" system call which is
> not used by userspace application code directly, it is impractical and
> there's very little point in forcing all userspace applications to
> explicitly allow it in order to avoid crashing tracked processes.
> 
> Pass this systemcall through seccomp without depending on configuration.
> 
> Note: uretprobe isn't supported in i386 and __NR_ia32_rt_tgsigqueueinfo
> uses the same number as __NR_uretprobe so the syscall isn't forced in the
> compat bitmap.
> 
> Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> Reported-by: Rafael Buchbinder <rafi@rbk.io>
> Link: https://lore.kernel.org/lkml/CAHsH6Gs3Eh8DFU0wq58c_LF8A4_+o6z456J7BidmcVY2AqOnHQ@mail.gmail.com/
> Link: https://lore.kernel.org/lkml/20250121182939.33d05470@gandalf.local.home/T/#me2676c378eff2d6a33f3054fed4a5f3afa64e65b
> Link: https://lore.kernel.org/lkml/20250128145806.1849977-1-eyal.birger@gmail.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> ---
> v3: no change - deferring 32bit compat handling as there aren't plans to
>     support this syscall in compat mode.
> v2: use action_cache bitmap and mode1 array to check the syscall
> ---
>  kernel/seccomp.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f59381c4a2ff..09b6f8e6db51 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -734,13 +734,13 @@ seccomp_prepare_user_filter(const char __user *user_filter)
>  
>  #ifdef SECCOMP_ARCH_NATIVE
>  /**
> - * seccomp_is_const_allow - check if filter is constant allow with given data
> + * seccomp_is_filter_const_allow - check if filter is constant allow with given data
>   * @fprog: The BPF programs
>   * @sd: The seccomp data to check against, only syscall number and arch
>   *      number are considered constant.
>   */
> -static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> -				   struct seccomp_data *sd)
> +static bool seccomp_is_filter_const_allow(struct sock_fprog_kern *fprog,
> +					  struct seccomp_data *sd)
>  {
>  	unsigned int reg_value = 0;
>  	unsigned int pc;
> @@ -812,6 +812,21 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
>  	return false;
>  }
>  
> +static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
> +				   struct seccomp_data *sd)
> +{
> +#ifdef __NR_uretprobe
> +	if (sd->nr == __NR_uretprobe
> +#ifdef SECCOMP_ARCH_COMPAT
> +	    && sd->arch != SECCOMP_ARCH_COMPAT
> +#endif
> +	   )
> +		return true;
> +#endif
> +
> +	return seccomp_is_filter_const_allow(fprog, sd);
> +}
> +
>  static void seccomp_cache_prepare_bitmap(struct seccomp_filter *sfilter,
>  					 void *bitmap, const void *bitmap_prev,
>  					 size_t bitmap_size, int arch)

I minimized the above to:

@@ -749,6 +749,15 @@ static bool seccomp_is_const_allow(struct sock_fprog_kern *fprog,
 	if (WARN_ON_ONCE(!fprog))
 		return false;
 
+	/* Our single exception to filtering. */
+#ifdef __NR_uretprobe
+#ifdef SECCOMP_ARCH_COMPAT
+	if (sd->arch == SECCOMP_ARCH_NATIVE)
+#endif
+		if (sd->nr == __NR_uretprobe)
+			return true;
+#endif
+
 	for (pc = 0; pc < fprog->len; pc++) {
 		struct sock_filter *insn = &fprog->filter[pc];
 		u16 code = insn->code;

> @@ -1023,6 +1038,9 @@ static inline void seccomp_log(unsigned long syscall, long signr, u32 action,
>   */
>  static const int mode1_syscalls[] = {
>  	__NR_seccomp_read, __NR_seccomp_write, __NR_seccomp_exit, __NR_seccomp_sigreturn,
> +#ifdef __NR_uretprobe
> +	__NR_uretprobe,
> +#endif
>  	-1, /* negative terminated */
>  };
>  
> -- 
> 2.43.0
> 

-Kees

-- 
Kees Cook

[PATCH v3 2/2] selftests/seccomp: validate uretprobe syscall passes through seccomp

[Eyal Birger]

The uretprobe syscall is implemented as a performance enhancement on
x86_64 by having the kernel inject a call to it on function exit; User
programs cannot call this system call explicitly.

As such, this syscall is considered a kernel implementation detail and
should not be filtered by seccomp.

Enhance the seccomp bpf test suite to check that uretprobes can be
attached to processes without the killing the process regardless of
seccomp policy.

Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
---
 tools/testing/selftests/seccomp/seccomp_bpf.c | 195 ++++++++++++++++++
 1 file changed, 195 insertions(+)

diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index 8c3a73461475..bee4f424c5c3 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -47,6 +47,7 @@
 #include <linux/kcmp.h>
 #include <sys/resource.h>
 #include <sys/capability.h>
+#include <linux/perf_event.h>
 
 #include <unistd.h>
 #include <sys/syscall.h>
@@ -68,6 +69,10 @@
 # define PR_SET_PTRACER 0x59616d61
 #endif
 
+#ifndef noinline
+#define noinline __attribute__((noinline))
+#endif
+
 #ifndef PR_SET_NO_NEW_PRIVS
 #define PR_SET_NO_NEW_PRIVS 38
 #define PR_GET_NO_NEW_PRIVS 39
@@ -4888,6 +4893,196 @@ TEST(tsync_vs_dead_thread_leader)
 	EXPECT_EQ(0, status);
 }
 
+noinline int probed(void)
+{
+	return 1;
+}
+
+static int parse_uint_from_file(const char *file, const char *fmt)
+{
+	int err = -1, ret;
+	FILE *f;
+
+	f = fopen(file, "re");
+	if (f) {
+		err = fscanf(f, fmt, &ret);
+		fclose(f);
+	}
+	return err == 1 ? ret : err;
+}
+
+static int determine_uprobe_perf_type(void)
+{
+	const char *file = "/sys/bus/event_source/devices/uprobe/type";
+
+	return parse_uint_from_file(file, "%d\n");
+}
+
+static int determine_uprobe_retprobe_bit(void)
+{
+	const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
+
+	return parse_uint_from_file(file, "config:%d\n");
+}
+
+static ssize_t get_uprobe_offset(const void *addr)
+{
+	size_t start, base, end;
+	bool found = false;
+	char buf[256];
+	FILE *f;
+
+	f = fopen("/proc/self/maps", "r");
+	if (!f)
+		return -1;
+
+	while (fscanf(f, "%zx-%zx %s %zx %*[^\n]\n", &start, &end, buf, &base) == 4) {
+		if (buf[2] == 'x' && (uintptr_t)addr >= start && (uintptr_t)addr < end) {
+			found = true;
+			break;
+		}
+	}
+	fclose(f);
+	return found ? (uintptr_t)addr - start + base : -1;
+}
+
+FIXTURE(URETPROBE) {
+	int fd;
+};
+
+FIXTURE_VARIANT(URETPROBE) {
+	/*
+	 * All of the URETPROBE behaviors can be tested with either
+	 * uretprobe attached or not
+	 */
+	bool attach;
+};
+
+FIXTURE_VARIANT_ADD(URETPROBE, attached) {
+	.attach = true,
+};
+
+FIXTURE_VARIANT_ADD(URETPROBE, not_attached) {
+	.attach = false,
+};
+
+FIXTURE_SETUP(URETPROBE)
+{
+	const size_t attr_sz = sizeof(struct perf_event_attr);
+	struct perf_event_attr attr;
+	ssize_t offset;
+	int type, bit;
+
+	if (!variant->attach)
+		return;
+
+	memset(&attr, 0, attr_sz);
+
+	type = determine_uprobe_perf_type();
+	ASSERT_GE(type, 0);
+	bit = determine_uprobe_retprobe_bit();
+	ASSERT_GE(bit, 0);
+	offset = get_uprobe_offset(probed);
+	ASSERT_GE(offset, 0);
+
+	attr.config |= 1 << bit;
+	attr.size = attr_sz;
+	attr.type = type;
+	attr.config1 = ptr_to_u64("/proc/self/exe");
+	attr.config2 = offset;
+
+	self->fd = syscall(__NR_perf_event_open, &attr,
+			   getpid() /* pid */, -1 /* cpu */, -1 /* group_fd */,
+			   PERF_FLAG_FD_CLOEXEC);
+}
+
+FIXTURE_TEARDOWN(URETPROBE)
+{
+	/* we could call close(self->fd), but we'd need extra filter for
+	 * that and since we are calling _exit right away..
+	 */
+}
+
+static int run_probed_with_filter(struct sock_fprog *prog)
+{
+	if (prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0) ||
+	    seccomp(SECCOMP_SET_MODE_FILTER, 0, prog)) {
+		return -1;
+	}
+
+	probed();
+	return 0;
+}
+
+TEST_F(URETPROBE, uretprobe_default_allow)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+
+	ASSERT_EQ(0, run_probed_with_filter(&prog));
+}
+
+TEST_F(URETPROBE, uretprobe_default_block)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+			offsetof(struct seccomp_data, nr)),
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+
+	ASSERT_EQ(0, run_probed_with_filter(&prog));
+}
+
+TEST_F(URETPROBE, uretprobe_block_uretprobe_syscall)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+			offsetof(struct seccomp_data, nr)),
+#ifdef __NR_uretprobe
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 0, 1),
+#endif
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+
+	ASSERT_EQ(0, run_probed_with_filter(&prog));
+}
+
+TEST_F(URETPROBE, uretprobe_default_block_with_uretprobe_syscall)
+{
+	struct sock_filter filter[] = {
+		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
+			offsetof(struct seccomp_data, nr)),
+#ifdef __NR_uretprobe
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 2, 0),
+#endif
+		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
+		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
+	};
+	struct sock_fprog prog = {
+		.len = (unsigned short)ARRAY_SIZE(filter),
+		.filter = filter,
+	};
+
+	ASSERT_EQ(0, run_probed_with_filter(&prog));
+}
+
 /*
  * TODO:
  * - expand NNP testing
-- 
2.43.0

Re: [PATCH v3 2/2] selftests/seccomp: validate uretprobe syscall passes through seccomp

[Jiri Olsa]

On Sun, Feb 02, 2025 at 08:29:21AM -0800, Eyal Birger wrote:

SNIP

> +TEST_F(URETPROBE, uretprobe_default_block)
> +{
> +	struct sock_filter filter[] = {
> +		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> +			offsetof(struct seccomp_data, nr)),
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> +	};
> +	struct sock_fprog prog = {
> +		.len = (unsigned short)ARRAY_SIZE(filter),
> +		.filter = filter,
> +	};
> +
> +	ASSERT_EQ(0, run_probed_with_filter(&prog));
> +}
> +
> +TEST_F(URETPROBE, uretprobe_block_uretprobe_syscall)
> +{
> +	struct sock_filter filter[] = {
> +		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> +			offsetof(struct seccomp_data, nr)),
> +#ifdef __NR_uretprobe
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 0, 1),
> +#endif

does it make sense to run these tests on archs without __NR_uretprobe ?

jirka

> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> +	};
> +	struct sock_fprog prog = {
> +		.len = (unsigned short)ARRAY_SIZE(filter),
> +		.filter = filter,
> +	};
> +
> +	ASSERT_EQ(0, run_probed_with_filter(&prog));
> +}
> +
> +TEST_F(URETPROBE, uretprobe_default_block_with_uretprobe_syscall)
> +{
> +	struct sock_filter filter[] = {
> +		BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> +			offsetof(struct seccomp_data, nr)),
> +#ifdef __NR_uretprobe
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 2, 0),
> +#endif
> +		BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
> +		BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> +	};
> +	struct sock_fprog prog = {
> +		.len = (unsigned short)ARRAY_SIZE(filter),
> +		.filter = filter,
> +	};
> +
> +	ASSERT_EQ(0, run_probed_with_filter(&prog));
> +}
> +
>  /*
>   * TODO:
>   * - expand NNP testing
> -- 
> 2.43.0
> 

Re: [PATCH v3 2/2] selftests/seccomp: validate uretprobe syscall passes through seccomp

[Eyal Birger]

On Sun, Feb 2, 2025 at 12:51 PM Jiri Olsa <olsajiri@gmail.com> wrote:
>
> On Sun, Feb 02, 2025 at 08:29:21AM -0800, Eyal Birger wrote:
>
> SNIP
>
> > +TEST_F(URETPROBE, uretprobe_default_block)
> > +{
> > +     struct sock_filter filter[] = {
> > +             BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> > +                     offsetof(struct seccomp_data, nr)),
> > +             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
> > +             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
> > +             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> > +     };
> > +     struct sock_fprog prog = {
> > +             .len = (unsigned short)ARRAY_SIZE(filter),
> > +             .filter = filter,
> > +     };
> > +
> > +     ASSERT_EQ(0, run_probed_with_filter(&prog));
> > +}
> > +
> > +TEST_F(URETPROBE, uretprobe_block_uretprobe_syscall)
> > +{
> > +     struct sock_filter filter[] = {
> > +             BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> > +                     offsetof(struct seccomp_data, nr)),
> > +#ifdef __NR_uretprobe
> > +             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 0, 1),
> > +#endif
>
> does it make sense to run these tests on archs without __NR_uretprobe ?

I considered ifdefing them out, but then thought that given it's not
a lot of code it'd be better for the tests to be compiling and
ready in case support is added on a new platform than to have to
worry about that at that point.

Eyal.

Re: [PATCH v3 2/2] selftests/seccomp: validate uretprobe syscall passes through seccomp

[Kees Cook]

On Sun, Feb 02, 2025 at 01:13:28PM -0800, Eyal Birger wrote:
> On Sun, Feb 2, 2025 at 12:51 PM Jiri Olsa <olsajiri@gmail.com> wrote:
> >
> > On Sun, Feb 02, 2025 at 08:29:21AM -0800, Eyal Birger wrote:
> >
> > SNIP
> >
> > > +TEST_F(URETPROBE, uretprobe_default_block)
> > > +{
> > > +     struct sock_filter filter[] = {
> > > +             BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> > > +                     offsetof(struct seccomp_data, nr)),
> > > +             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_exit_group, 1, 0),
> > > +             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL),
> > > +             BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW),
> > > +     };
> > > +     struct sock_fprog prog = {
> > > +             .len = (unsigned short)ARRAY_SIZE(filter),
> > > +             .filter = filter,
> > > +     };
> > > +
> > > +     ASSERT_EQ(0, run_probed_with_filter(&prog));
> > > +}
> > > +
> > > +TEST_F(URETPROBE, uretprobe_block_uretprobe_syscall)
> > > +{
> > > +     struct sock_filter filter[] = {
> > > +             BPF_STMT(BPF_LD|BPF_W|BPF_ABS,
> > > +                     offsetof(struct seccomp_data, nr)),
> > > +#ifdef __NR_uretprobe
> > > +             BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_uretprobe, 0, 1),
> > > +#endif
> >
> > does it make sense to run these tests on archs without __NR_uretprobe ?
> 
> I considered ifdefing them out, but then thought that given it's not
> a lot of code it'd be better for the tests to be compiling and
> ready in case support is added on a new platform than to have to
> worry about that at that point.

The trouble I had is that on other archs, the tests fail. I've added
this, which retains build coverage, but doesn't trigger failures without
__NR_uretprobe:


diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c
index bee4f424c5c3..14ba51b52095 100644
--- a/tools/testing/selftests/seccomp/seccomp_bpf.c
+++ b/tools/testing/selftests/seccomp/seccomp_bpf.c
@@ -4973,6 +4973,10 @@ FIXTURE_SETUP(URETPROBE)
 	ssize_t offset;
 	int type, bit;
 
+#ifndef __NR_uretprobe
+	SKIP(return, "__NR_uretprobe syscall not defined");
+#endif
+
 	if (!variant->attach)
 		return;
 

-- 
Kees Cook

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Kees Cook]

On Sun, 02 Feb 2025 08:29:19 -0800, Eyal Birger wrote:
> uretprobe(2) is an performance enhancement system call added to improve
> uretprobes on x86_64.
> 
> Confinement environments such as Docker are not aware of this new system
> call and kill confined processes when uretprobes are attached to them.
> 
> Since uretprobe is a "kernel implementation detail" system call which is
> not used by userspace application code directly, pass this system call
> through seccomp without forcing existing userspace confinement environments
> to be changed.
> 
> [...]

With the changes I mentioned in each patch, I've applied this to
for-next/seccomp, with the intention of getting them into v6.14-rc2.

Thanks!

[1/2] seccomp: passthrough uretprobe systemcall without filtering
      https://git.kernel.org/kees/c/cf6cb56ef244
[2/2] selftests/seccomp: validate uretprobe syscall passes through seccomp
      https://git.kernel.org/kees/c/c2debdb8544f

Take care,

-- 
Kees Cook

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Eyal Birger]

On Thu, Feb 6, 2025 at 1:22 PM Kees Cook <kees@kernel.org> wrote:
>
> On Sun, 02 Feb 2025 08:29:19 -0800, Eyal Birger wrote:
> > uretprobe(2) is an performance enhancement system call added to improve
> > uretprobes on x86_64.
> >
> > Confinement environments such as Docker are not aware of this new system
> > call and kill confined processes when uretprobes are attached to them.
> >
> > Since uretprobe is a "kernel implementation detail" system call which is
> > not used by userspace application code directly, pass this system call
> > through seccomp without forcing existing userspace confinement environments
> > to be changed.
> >
> > [...]
>
> With the changes I mentioned in each patch, I've applied this to
> for-next/seccomp, with the intention of getting them into v6.14-rc2.
>
> Thanks!

Thank you very much for your help.

Eyal.

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Jiri Olsa]

On Thu, Feb 06, 2025 at 05:06:29PM -0800, Eyal Birger wrote:
> On Thu, Feb 6, 2025 at 1:22 PM Kees Cook <kees@kernel.org> wrote:
> >
> > On Sun, 02 Feb 2025 08:29:19 -0800, Eyal Birger wrote:
> > > uretprobe(2) is an performance enhancement system call added to improve
> > > uretprobes on x86_64.
> > >
> > > Confinement environments such as Docker are not aware of this new system
> > > call and kill confined processes when uretprobes are attached to them.
> > >
> > > Since uretprobe is a "kernel implementation detail" system call which is
> > > not used by userspace application code directly, pass this system call
> > > through seccomp without forcing existing userspace confinement environments
> > > to be changed.
> > >
> > > [...]
> >
> > With the changes I mentioned in each patch, I've applied this to
> > for-next/seccomp, with the intention of getting them into v6.14-rc2.
> >
> > Thanks!
> 
> Thank you very much for your help.

great!

thanks,
jirka

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Jann Horn]

On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> uretprobe(2) is an performance enhancement system call added to improve
> uretprobes on x86_64.
>
> Confinement environments such as Docker are not aware of this new system
> call and kill confined processes when uretprobes are attached to them.

FYI, you might have similar issues with Syscall User Dispatch
(https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
potentially also with ptrace-based sandboxes, depending on what kinda
processes you inject uprobes into. For Syscall User Dispatch, there is
already precedent for a bypass based on instruction pointer (see
syscall_user_dispatch()).

> Since uretprobe is a "kernel implementation detail" system call which is
> not used by userspace application code directly, pass this system call
> through seccomp without forcing existing userspace confinement environments
> to be changed.

This makes me feel kinda uncomfortable. The purpose of seccomp() is
that you can create a process that is as locked down as you want; you
can use it for some light limits on what a process can do (like in
Docker), or you can use it to make a process that has access to
essentially nothing except read(), write() and exit_group(). Even
stuff like restart_syscall() and rt_sigreturn() is not currently
excepted from that.

I guess your usecase is a little special in that you were already
calling from userspace into the kernel with SWBP before, which is also
not subject to seccomp; and the syscall is essentially an
arch-specific hack to make the SWBP a little faster.

If we do this, we should at least ensure that there is absolutely no
way for anything to happen in sys_uretprobe when no uretprobes are
configured for the process - the first check in the syscall
implementation almost does that, but the implementation could be a bit
stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
uprobe region exists for the process, trampoline_check_ip() returns
`-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
there is a userspace instruction pointer near the bottom of the
address space that is allowed to call into the syscall if uretprobes
are not set up. Though the mmap minimum address restrictions will
typically prevent creating mappings there, and
uprobe_handle_trampoline() will SIGILL us if we get that far without a
valid uretprobe.

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Eyal Birger]

Hi,

On Fri, Feb 7, 2025 at 7:27 AM Jann Horn <jannh@google.com> wrote:
>
> On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> > uretprobe(2) is an performance enhancement system call added to improve
> > uretprobes on x86_64.
> >
> > Confinement environments such as Docker are not aware of this new system
> > call and kill confined processes when uretprobes are attached to them.
>
> FYI, you might have similar issues with Syscall User Dispatch
> (https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
> potentially also with ptrace-based sandboxes, depending on what kinda
> processes you inject uprobes into. For Syscall User Dispatch, there is
> already precedent for a bypass based on instruction pointer (see
> syscall_user_dispatch()).

Thanks. This is interesting.

Do you know of confinement environments using this?

>
> > Since uretprobe is a "kernel implementation detail" system call which is
> > not used by userspace application code directly, pass this system call
> > through seccomp without forcing existing userspace confinement environments
> > to be changed.
>
> This makes me feel kinda uncomfortable. The purpose of seccomp() is
> that you can create a process that is as locked down as you want; you
> can use it for some light limits on what a process can do (like in
> Docker), or you can use it to make a process that has access to
> essentially nothing except read(), write() and exit_group(). Even
> stuff like restart_syscall() and rt_sigreturn() is not currently
> excepted from that.

Yes, this has been discussed at length in the threads mentioned
in the "Link" tags.

>
> I guess your usecase is a little special in that you were already
> calling from userspace into the kernel with SWBP before, which is also
> not subject to seccomp; and the syscall is essentially an
> arch-specific hack to make the SWBP a little faster.

Indeed. The uretprobe mechanism wasn't enforced by seccomp before
this syscall. This change preserves this.

>
> If we do this, we should at least ensure that there is absolutely no
> way for anything to happen in sys_uretprobe when no uretprobes are
> configured for the process - the first check in the syscall
> implementation almost does that, but the implementation could be a bit
> stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
> uprobe region exists for the process, trampoline_check_ip() returns
> `-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
> there is a userspace instruction pointer near the bottom of the
> address space that is allowed to call into the syscall if uretprobes
> are not set up. Though the mmap minimum address restrictions will
> typically prevent creating mappings there, and
> uprobe_handle_trampoline() will SIGILL us if we get that far without a
> valid uretprobe.

I'm not sure I understand your point. If creating mappings in that
area is prevented, what is the issue? also, this would be related to the
uretprobe syscall implementation in general, no?

To me this seems unrelated to the seccomp change.
Jiri, do you have any input on this?

Thanks!
Eyal.

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Jann Horn]

On Fri, Feb 7, 2025 at 5:20 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> On Fri, Feb 7, 2025 at 7:27 AM Jann Horn <jannh@google.com> wrote:
> >
> > On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> > > uretprobe(2) is an performance enhancement system call added to improve
> > > uretprobes on x86_64.
> > >
> > > Confinement environments such as Docker are not aware of this new system
> > > call and kill confined processes when uretprobes are attached to them.
> >
> > FYI, you might have similar issues with Syscall User Dispatch
> > (https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
> > potentially also with ptrace-based sandboxes, depending on what kinda
> > processes you inject uprobes into. For Syscall User Dispatch, there is
> > already precedent for a bypass based on instruction pointer (see
> > syscall_user_dispatch()).
>
> Thanks. This is interesting.
>
> Do you know of confinement environments using this?

Not for Syscall User Dispatch; I think that was mostly intended for
stuff like emulating Windows syscalls in WINE. I'm not sure who
actually uses it, I just know a bit about the kernel side of it.

From what I know, ptrace sandboxing is a technique used by some
configurations of gVisor
(https://gvisor.dev/docs/architecture_guide/platforms/#ptrace), though
now I see that that page says that this configuration is no longer
supported. I am also not sure whether you'd ever have uprobes
installed in files from which instructions are executed in this
environment.

> > > Since uretprobe is a "kernel implementation detail" system call which is
> > > not used by userspace application code directly, pass this system call
> > > through seccomp without forcing existing userspace confinement environments
> > > to be changed.
> >
> > This makes me feel kinda uncomfortable. The purpose of seccomp() is
> > that you can create a process that is as locked down as you want; you
> > can use it for some light limits on what a process can do (like in
> > Docker), or you can use it to make a process that has access to
> > essentially nothing except read(), write() and exit_group(). Even
> > stuff like restart_syscall() and rt_sigreturn() is not currently
> > excepted from that.
>
> Yes, this has been discussed at length in the threads mentioned
> in the "Link" tags.
>
> >
> > I guess your usecase is a little special in that you were already
> > calling from userspace into the kernel with SWBP before, which is also
> > not subject to seccomp; and the syscall is essentially an
> > arch-specific hack to make the SWBP a little faster.
>
> Indeed. The uretprobe mechanism wasn't enforced by seccomp before
> this syscall. This change preserves this.
>
> >
> > If we do this, we should at least ensure that there is absolutely no
> > way for anything to happen in sys_uretprobe when no uretprobes are
> > configured for the process - the first check in the syscall
> > implementation almost does that, but the implementation could be a bit
> > stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
> > uprobe region exists for the process, trampoline_check_ip() returns
> > `-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
> > there is a userspace instruction pointer near the bottom of the
> > address space that is allowed to call into the syscall if uretprobes
> > are not set up. Though the mmap minimum address restrictions will
> > typically prevent creating mappings there, and
> > uprobe_handle_trampoline() will SIGILL us if we get that far without a
> > valid uretprobe.
>
> I'm not sure I understand your point. If creating mappings in that
> area is prevented, what is the issue?

It is usually prevented, not always - root can do it depending on
system configuration.

Also, in a syscall like this that will be reachable in every sandbox,
I think we should try to be more careful about edge cases and avoid
things like this offset calculation on address -1.

> also, this would be related to the
> uretprobe syscall implementation in general, no?

Yes. I just think it is relevant to the seccomp change because
excepting a syscall from seccomp makes it more important that that
syscall is robust and correct.

> To me this seems unrelated to the seccomp change.
> Jiri, do you have any input on this?
>
> Thanks!
> Eyal.

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Jiri Olsa]

On Fri, Feb 07, 2025 at 04:27:09PM +0100, Jann Horn wrote:
> On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> > uretprobe(2) is an performance enhancement system call added to improve
> > uretprobes on x86_64.
> >
> > Confinement environments such as Docker are not aware of this new system
> > call and kill confined processes when uretprobes are attached to them.
> 
> FYI, you might have similar issues with Syscall User Dispatch
> (https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
> potentially also with ptrace-based sandboxes, depending on what kinda
> processes you inject uprobes into. For Syscall User Dispatch, there is
> already precedent for a bypass based on instruction pointer (see
> syscall_user_dispatch()).
> 
> > Since uretprobe is a "kernel implementation detail" system call which is
> > not used by userspace application code directly, pass this system call
> > through seccomp without forcing existing userspace confinement environments
> > to be changed.
> 
> This makes me feel kinda uncomfortable. The purpose of seccomp() is
> that you can create a process that is as locked down as you want; you
> can use it for some light limits on what a process can do (like in
> Docker), or you can use it to make a process that has access to
> essentially nothing except read(), write() and exit_group(). Even
> stuff like restart_syscall() and rt_sigreturn() is not currently
> excepted from that.
> 
> I guess your usecase is a little special in that you were already
> calling from userspace into the kernel with SWBP before, which is also
> not subject to seccomp; and the syscall is essentially an
> arch-specific hack to make the SWBP a little faster.
> 
> If we do this, we should at least ensure that there is absolutely no
> way for anything to happen in sys_uretprobe when no uretprobes are
> configured for the process - the first check in the syscall
> implementation almost does that, but the implementation could be a bit
> stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
> uprobe region exists for the process, trampoline_check_ip() returns
> `-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
> there is a userspace instruction pointer near the bottom of the
> address space that is allowed to call into the syscall if uretprobes
> are not set up. Though the mmap minimum address restrictions will
> typically prevent creating mappings there, and
> uprobe_handle_trampoline() will SIGILL us if we get that far without a
> valid uretprobe.

nice catch, I think change below should fix that

thanks,
jirka


---
diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 0c74a4d4df65..9b8837d8f06e 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -368,19 +368,21 @@ void *arch_uretprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
-static unsigned long trampoline_check_ip(void)
+static unsigned long trampoline_check_ip(unsigned long tramp)
 {
-	unsigned long tramp = uprobe_get_trampoline_vaddr();
-
 	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
 }
 
 SYSCALL_DEFINE0(uretprobe)
 {
 	struct pt_regs *regs = task_pt_regs(current);
-	unsigned long err, ip, sp, r11_cx_ax[3];
+	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
+
+	tramp = uprobe_get_trampoline_vaddr();
+	if (tramp == -1)
+		goto sigill;
 
-	if (regs->ip != trampoline_check_ip())
+	if (regs->ip != trampoline_check_ip(tramp))
 		goto sigill;
 
 	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));

Re: [PATCH v3 0/2] seccomp: pass uretprobe system call through seccomp

[Kees Cook]

On Sat, Feb 08, 2025 at 01:03:55AM +0100, Jiri Olsa wrote:
> On Fri, Feb 07, 2025 at 04:27:09PM +0100, Jann Horn wrote:
> > On Sun, Feb 2, 2025 at 5:29 PM Eyal Birger <eyal.birger@gmail.com> wrote:
> > > uretprobe(2) is an performance enhancement system call added to improve
> > > uretprobes on x86_64.
> > >
> > > Confinement environments such as Docker are not aware of this new system
> > > call and kill confined processes when uretprobes are attached to them.
> > 
> > FYI, you might have similar issues with Syscall User Dispatch
> > (https://docs.kernel.org/admin-guide/syscall-user-dispatch.html) and
> > potentially also with ptrace-based sandboxes, depending on what kinda
> > processes you inject uprobes into. For Syscall User Dispatch, there is
> > already precedent for a bypass based on instruction pointer (see
> > syscall_user_dispatch()).
> > 
> > > Since uretprobe is a "kernel implementation detail" system call which is
> > > not used by userspace application code directly, pass this system call
> > > through seccomp without forcing existing userspace confinement environments
> > > to be changed.
> > 
> > This makes me feel kinda uncomfortable. The purpose of seccomp() is
> > that you can create a process that is as locked down as you want; you
> > can use it for some light limits on what a process can do (like in
> > Docker), or you can use it to make a process that has access to
> > essentially nothing except read(), write() and exit_group(). Even
> > stuff like restart_syscall() and rt_sigreturn() is not currently
> > excepted from that.
> > 
> > I guess your usecase is a little special in that you were already
> > calling from userspace into the kernel with SWBP before, which is also
> > not subject to seccomp; and the syscall is essentially an
> > arch-specific hack to make the SWBP a little faster.
> > 
> > If we do this, we should at least ensure that there is absolutely no
> > way for anything to happen in sys_uretprobe when no uretprobes are
> > configured for the process - the first check in the syscall
> > implementation almost does that, but the implementation could be a bit
> > stricter. It checks for "regs->ip != trampoline_check_ip()", but if no
> > uprobe region exists for the process, trampoline_check_ip() returns
> > `-1 + (uretprobe_syscall_check - uretprobe_trampoline_entry)`. So
> > there is a userspace instruction pointer near the bottom of the
> > address space that is allowed to call into the syscall if uretprobes
> > are not set up. Though the mmap minimum address restrictions will
> > typically prevent creating mappings there, and
> > uprobe_handle_trampoline() will SIGILL us if we get that far without a
> > valid uretprobe.
> 
> nice catch, I think change below should fix that

Thanks! Please backport this to -stable too. :)

-Kees

> 
> thanks,
> jirka
> 
> 
> ---
> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 0c74a4d4df65..9b8837d8f06e 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -368,19 +368,21 @@ void *arch_uretprobe_trampoline(unsigned long *psize)
>  	return &insn;
>  }
>  
> -static unsigned long trampoline_check_ip(void)
> +static unsigned long trampoline_check_ip(unsigned long tramp)
>  {
> -	unsigned long tramp = uprobe_get_trampoline_vaddr();
> -
>  	return tramp + (uretprobe_syscall_check - uretprobe_trampoline_entry);
>  }
>  
>  SYSCALL_DEFINE0(uretprobe)
>  {
>  	struct pt_regs *regs = task_pt_regs(current);
> -	unsigned long err, ip, sp, r11_cx_ax[3];
> +	unsigned long err, ip, sp, r11_cx_ax[3], tramp;
> +
> +	tramp = uprobe_get_trampoline_vaddr();
> +	if (tramp == -1)
> +		goto sigill;
>  
> -	if (regs->ip != trampoline_check_ip())
> +	if (regs->ip != trampoline_check_ip(tramp))
>  		goto sigill;
>  
>  	err = copy_from_user(r11_cx_ax, (void __user *)regs->sp, sizeof(r11_cx_ax));

-- 
Kees Cook