Re: [PATCH v2] seccomp: passthrough uretprobe systemcall without filtering
From: Kees Cook <kees@kernel.org>
Date: 2025-01-29 01:41:41
Also in:
bpf, linux-api, lkml, stable
On Tue, Jan 28, 2025 at 06:58:06AM -0800, Eyal Birger wrote:
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.
So a 64-bit tracer cannot use uretprobe on a 32-bit process? Also is uretprobe strictly an x86_64 feature?
quoted hunk ↗ jump to hunk
[...]diff --git a/kernel/seccomp.c b/kernel/seccomp.c index 385d48293a5f..23b594a68bc0 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
I don't like this because it's not future-proof enough. __NR_uretprobe
may collide with other syscalls at some point. And if __NR_uretprobe_32
is ever implemented, the seccomp logic will be missing. I think this
will work now and in the future:
#ifdef __NR_uretprobe
# ifdef SECCOMP_ARCH_COMPAT
if (sd->arch == SECCOMP_ARCH_COMPAT) {
# ifdef __NR_uretprobe_32
if (sd->nr == __NR_uretprobe_32)
return true;
# endif
} else
# endif
if (sd->nr == __NR_uretprobe)
return true;
#endif
Instead of doing a function rename dance, I think you can just stick
the above into seccomp_is_const_allow() after the WARN().
Also please add a KUnit tests to cover this in
tools/testing/selftests/seccomp/seccomp_bpf.c
With at least these cases combinations below. Check each of:
- not using uretprobe passes
- using uretprobe passes (and validates that uretprobe did work)
in each of the following conditions:
- default-allow filter
- default-block filter
- filter explicitly blocking __NR_uretprobe and nothing else
- filter explicitly allowing __NR_uretprobe (and only other
required syscalls)
Hm, is uretprobe expected to work on mips? Because if so, you'll need to
do something similar to the mode1 checking in the !SECCOMP_ARCH_NATIVE
version of seccomp_cache_check_allow().
(You can see why I really dislike having policy baked into seccomp!)
quoted hunk ↗ jump to hunk
+ ) + 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
It'd be nice to update mode1_syscalls_32 with __NR_uretprobe_32 even though it doesn't exist. (Is it _never_ planned to be implemented?) But then, maybe the chances of a compat mode1 seccomp process running under uretprobe is vanishingly small.
-1, /* negative terminated */ }; -- 2.43.0
-Kees -- Kees Cook