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_+o6z456J7BidmcVY2Aq... Link: https://lore.kernel.org/lkml/20250121182939.33d05470@gandalf.local.home/T/#m... Cc: stable@vger.kernel.org Signed-off-by: Eyal Birger eyal.birger@gmail.com --- v2: use action_cache bitmap and mode1 array to check the syscall
The following reproduction script synthetically demonstrates the problem for seccomp filters:
cat > /tmp/x.c << EOF
char *syscalls[] = { "write", "exit_group", "fstat", };
__attribute__((noinline)) int probed(void) { printf("Probed\n"); return 1; }
void apply_seccomp_filter(char **syscalls, int num_syscalls) { scmp_filter_ctx ctx;
ctx = seccomp_init(SCMP_ACT_KILL); for (int i = 0; i < num_syscalls; i++) { seccomp_rule_add(ctx, SCMP_ACT_ALLOW, seccomp_syscall_resolve_name(syscalls[i]), 0); } seccomp_load(ctx); seccomp_release(ctx); }
int main(int argc, char *argv[]) { int num_syscalls = sizeof(syscalls) / sizeof(syscalls[0]);
apply_seccomp_filter(syscalls, num_syscalls);
probed();
return 0; } EOF
cat > /tmp/trace.bt << EOF uretprobe:/tmp/x:probed { printf("ret=%d\n", retval); } EOF
gcc -o /tmp/x /tmp/x.c -lseccomp
/usr/bin/bpftrace /tmp/trace.bt &
sleep 5 # wait for uretprobe attach /tmp/x
pkill bpftrace
rm /tmp/x /tmp/x.c /tmp/trace.bt --- kernel/seccomp.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)
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 + ) + 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 */ };
can't review, I know nothing about seccomp_cache, but
On 01/28, Eyal Birger wrote:
+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
it seems you can check
&& sd->arch == SECCOMP_ARCH_NATIVE
and avoid #ifdef SECCOMP_ARCH_COMPAT
Oleg.
On 01/28, Oleg Nesterov wrote:
can't review, I know nothing about seccomp_cache, but
On 01/28, Eyal Birger wrote:
+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
it seems you can check
&& sd->arch == SECCOMP_ARCH_NATIVEand avoid #ifdef SECCOMP_ARCH_COMPAT
Although perhaps you added this ifdef to avoid the unnecessary sd->arch check if !CONFIG_COMPAT ...
Oleg.
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?
[...] 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!)
)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
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
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?
My understanding is that they'd be able to do so, but use the int3 trap instead of the uretprobe syscall.
[...] 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.
I'm not sure I got this 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
I don't know if implementing uretprobe syscall for compat binaries is planned or makes sense - I'd appreciate Jiri's and others opinion on that. That said, I don't mind adding this code for the sake of future proofing.
Instead of doing a function rename dance, I think you can just stick the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add new syscalls to this as well, so I wanted to avoid cluttering the existing function which seems to be well defined.
Also please add a KUnit tests to cover this in tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)
Ok.
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().
I don't know if uretprobe syscall is expected to run on mips. Personally I'd avoid adding this dead code.
(You can see why I really dislike having policy baked into seccomp!)
I definitely understand :)
)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.
It seems to me very unlikely. BTW, when I tested the "strict" mode change my program was killed by seccomp. The reason wasn't the uretprobe syscall (which I added to the list), it was actually the exit_group syscall which libc uses instead of the exit syscall.
Thanks again, Eyal.
On Wed, Jan 29, 2025 at 9:27 AM Eyal Birger eyal.birger@gmail.com wrote:
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
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?
My understanding is that they'd be able to do so, but use the int3 trap instead of the uretprobe syscall.
Syscall-based uretprobe implementation is strictly x86-64 and I don't think we have any plans to expand it beyond x86-64. But uretprobes in general do work across many bitnesses and architectures, they are just implemented through a trap approach (int3 on x86), so none of that should be relevant to seccomp. It's just that trapping on x86-64 is that much slower that we had to do syscall to speed it up but quite a lot.
[...] 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)
[...]
On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
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?
My understanding is that they'd be able to do so, but use the int3 trap instead of the uretprobe syscall.
[...] 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.
I'm not sure I got this 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
I don't know if implementing uretprobe syscall for compat binaries is planned or makes sense - I'd appreciate Jiri's and others opinion on that. That said, I don't mind adding this code for the sake of future proofing.
as Andrii wrote in the other email ATM it's just strictly x86_64, but let's future proof it
AFAIK there was an attempt to do similar on arm but it did not show any speed up
Instead of doing a function rename dance, I think you can just stick the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add new syscalls to this as well, so I wanted to avoid cluttering the existing function which seems to be well defined.
Also please add a KUnit tests to cover this in tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)Ok.
please let me know if I can help in any way with tests
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().
I don't know if uretprobe syscall is expected to run on mips. Personally I'd avoid adding this dead code.
(You can see why I really dislike having policy baked into seccomp!)
I definitely understand :)
)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.
no plans for __NR_uretprobe_32 at this point
It seems to me very unlikely. BTW, when I tested the "strict" mode change my program was killed by seccomp. The reason wasn't the uretprobe syscall (which I added to the list), it was actually the exit_group syscall which libc uses instead of the exit syscall.
Thanks again, Eyal.
thanks, jirka
On Thu, Jan 30, 2025 at 12:24 AM Jiri Olsa olsajiri@gmail.com wrote:
On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
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?
My understanding is that they'd be able to do so, but use the int3 trap instead of the uretprobe syscall.
[...] 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.
I'm not sure I got this 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
I don't know if implementing uretprobe syscall for compat binaries is planned or makes sense - I'd appreciate Jiri's and others opinion on that. That said, I don't mind adding this code for the sake of future proofing.
as Andrii wrote in the other email ATM it's just strictly x86_64, but let's future proof it
Thank you. So I'm ok with using the suggestion above, but more on this below.
AFAIK there was an attempt to do similar on arm but it did not show any speed up
Instead of doing a function rename dance, I think you can just stick the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add new syscalls to this as well, so I wanted to avoid cluttering the existing function which seems to be well defined.
Also please add a KUnit tests to cover this in tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)Ok.
please let me know if I can help in any way with tests
Thanks! Is there a way to partition this work? I'd appreciate the help if we can find some way of doing so.
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().
I don't know if uretprobe syscall is expected to run on mips. Personally I'd avoid adding this dead code.
Jiri, what is your take on this one?
(You can see why I really dislike having policy baked into seccomp!)
I definitely understand :)
)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.
no plans for __NR_uretprobe_32 at this point
So if we go with the suggestion above, we'll support the theoretical __NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and that's ok because strict seccomp is less common?
Personally I'd prefer to limit the scope of this fix to the problem we are aware of, and not possible problems should someone decide to reimplement uretprobes on different archs in a different way. Especially as this fix needs to be backmerged to stable kernels. So my personal preference would be to avoid __NR_uretprobe_32 in this patch and deal with it if it ever gets implemented.
Thoughts and advice appreciated, Eyal.
On 01/30, Eyal Birger wrote:
Personally I'd prefer to limit the scope of this fix to the problem we are aware of, and not possible problems should someone decide to reimplement uretprobes on different archs in a different way. Especially as this fix needs to be backmerged to stable kernels.
+1
Oleg.
On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
So if we go with the suggestion above, we'll support the theoretical __NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and that's ok because strict seccomp is less common?
It's so uncommon I regularly consider removing it entirely. :)
Personally I'd prefer to limit the scope of this fix to the problem we are aware of, and not possible problems should someone decide to reimplement uretprobes on different archs in a different way. Especially as this fix needs to be backmerged to stable kernels. So my personal preference would be to avoid __NR_uretprobe_32 in this patch and deal with it if it ever gets implemented.
That's fine, but I want the exception to be designed to fail closed instead of failing open. I think my proposed future-proof check does this.
On Thu, Jan 30, 2025 at 7:57 AM Kees Cook kees@kernel.org wrote:
On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
So if we go with the suggestion above, we'll support the theoretical __NR_uretprobe_32 for filtered seccomp, but not for strict seccomp, and that's ok because strict seccomp is less common?
It's so uncommon I regularly consider removing it entirely. :)
Personally I'd prefer to limit the scope of this fix to the problem we are aware of, and not possible problems should someone decide to reimplement uretprobes on different archs in a different way. Especially as this fix needs to be backmerged to stable kernels. So my personal preference would be to avoid __NR_uretprobe_32 in this patch and deal with it if it ever gets implemented.
That's fine, but I want the exception to be designed to fail closed instead of failing open. I think my proposed future-proof check does this.
I think it does. I think the code in the patch does too, since it avoids the special handling for compat, so defaults to the existing behavior which blocks the syscall.
Eyal.
On Thu, Jan 30, 2025 at 07:05:42AM -0800, Eyal Birger wrote:
On Thu, Jan 30, 2025 at 12:24 AM Jiri Olsa olsajiri@gmail.com wrote:
On Wed, Jan 29, 2025 at 09:27:49AM -0800, Eyal Birger wrote:
Hi,
Thanks for the review!
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
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?
My understanding is that they'd be able to do so, but use the int3 trap instead of the uretprobe syscall.
[...] 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.
I'm not sure I got this 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
I don't know if implementing uretprobe syscall for compat binaries is planned or makes sense - I'd appreciate Jiri's and others opinion on that. That said, I don't mind adding this code for the sake of future proofing.
as Andrii wrote in the other email ATM it's just strictly x86_64, but let's future proof it
Thank you. So I'm ok with using the suggestion above, but more on this below.
AFAIK there was an attempt to do similar on arm but it did not show any speed up
Instead of doing a function rename dance, I think you can just stick the above into seccomp_is_const_allow() after the WARN().
My motivation for the renaming dance was that you mentioned we might add new syscalls to this as well, so I wanted to avoid cluttering the existing function which seems to be well defined.
Also please add a KUnit tests to cover this in tools/testing/selftests/seccomp/seccomp_bpf.c
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)Ok.
please let me know if I can help in any way with tests
Thanks! Is there a way to partition this work? I'd appreciate the help if we can find some way of doing so.
sure, I'll check the seccomp selftests and let you know
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().
I don't know if uretprobe syscall is expected to run on mips. Personally I'd avoid adding this dead code.
Jiri, what is your take on this one?
uretprobe syscall is not expected to work on mips, atm it's strictly x86_64
jirka
On Thu, Jan 30, 2025 at 10:53:47PM +0100, Jiri Olsa wrote:
SNIP
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)Ok.
please let me know if I can help in any way with tests
Thanks! Is there a way to partition this work? I'd appreciate the help if we can find some way of doing so.
sure, I'll check the seccomp selftests and let you know
hi, if it's any help, feel free to use the code below that creates uretprobe, it could be bit simpler if we use libbpf, but I think that's not an option
jirka
--- diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/testing/selftests/seccomp/seccomp_bpf.c index 8c3a73461475..1f99d31d05a1 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> @@ -4888,6 +4889,130 @@ TEST(tsync_vs_dead_thread_leader) EXPECT_EQ(0, status); }
+__attribute__((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; +} + +static int create_uretprobe(void *addr) +{ + const size_t attr_sz = sizeof(struct perf_event_attr); + struct perf_event_attr attr; + ssize_t offset; + int type, bit; + + memset(&attr, 0, attr_sz); + + type = determine_uprobe_perf_type(); + if (type < 0) + return -1; + bit = determine_uprobe_retprobe_bit(); + if (bit < 0) + return -1; + offset = get_uprobe_offset(probed); + if (offset < 0) + return -1; + + attr.config |= 1 << bit; + attr.size = attr_sz; + attr.type = type; + attr.config1 = ptr_to_u64("/proc/self/exe"); + attr.config2 = offset; + + return syscall(__NR_perf_event_open, &attr, + getpid() /* pid */, -1 /* cpu */, -1 /* group_fd */, + PERF_FLAG_FD_CLOEXEC); +} + +TEST(uretprobe) +{ + 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, + }; + long ret; + int fd; + + fd = create_uretprobe(probed); + ASSERT_GE(fd, 0) { + TH_LOG("Failed to create uretprobe!!"); + } + + ret = prctl(PR_SET_NO_NEW_PRIVS, 1, NULL, 0, 0); + ASSERT_EQ(0, ret) { + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); + } + + ret = seccomp(SECCOMP_SET_MODE_FILTER, 0, &prog); + ASSERT_NE(ENOSYS, errno) { + TH_LOG("Kernel does not support seccomp syscall!"); + } + EXPECT_EQ(0, ret) { + TH_LOG("Could not install filter!"); + } + + /* should not explode */ + probed(); + + /* we could call close(fd), but we'd need extra filter for + * that and since we we are calling _exit right away.. */ +} + /* * TODO: * - expand NNP testing
On Sun, Feb 2, 2025 at 3:08 AM Jiri Olsa olsajiri@gmail.com wrote:
On Thu, Jan 30, 2025 at 10:53:47PM +0100, Jiri Olsa wrote:
SNIP
I think this would mean that this test suite would need to run as privileged. Is that Ok? or maybe it'd be better to have a new suite?
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)Ok.
please let me know if I can help in any way with tests
Thanks! Is there a way to partition this work? I'd appreciate the help if we can find some way of doing so.
sure, I'll check the seccomp selftests and let you know
hi, if it's any help, feel free to use the code below that creates uretprobe, it could be bit simpler if we use libbpf, but I think that's not an option
Thank you very much! Eyal.
Hi Kees,
On Tue, Jan 28, 2025 at 5:41 PM Kees Cook kees@kernel.org wrote:
[...] 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)
In order to validate my understanding of the required test cases, I've attached a small bash script which validates them. As expected, the script fails without the suggested change.
If there are gaps in my understanding of the required scope, please let me know. I plan to port these test cases to use the kselftests infrastructure as requested.
To my understanding, the other issues with regards to the proposed patch are resolved, i.e. there aren't plans to support a 32 bit or mips flavor of this syscall, and the suggested patch fails closed if they are added.
As such, is it possible to merge the suggested patch so it could be back merged? I'm suggesting this in the interest of time, as for example Ubuntu LTS is going to be using kernel 6.11 soon [1] and other distributions are probably going to as well, and I believe the coding/review process for the testing code will take a while and probably won't be backmerged anyway.
Thanks! Eyal.
[1] https://www.omgubuntu.co.uk/2025/01/ubuntu-24-04-2-release-date
linux-stable-mirror@lists.linaro.org
-
Andrii Nakryiko -
Eyal Birger -
Jiri Olsa -
Kees Cook -
Oleg Nesterov