On Fri, Jan 17, 2025 at 12:02 AM Masami Hiramatsu <[email protected]> wrote: > > On Fri, 17 Jan 2025 02:39:28 +0100 > Oleg Nesterov <[email protected]> wrote: > > > On 01/16, Eyal Birger wrote: > > > > > > Fixes: ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return > > > probe") > > > Reported-by: Rafael Buchbinder <[email protected]> > > > Link: > > > https://lore.kernel.org/lkml/cahsh6gs3eh8dfu0wq58c_lf8a4_+o6z456j7bidmcvy2aqo...@mail.gmail.com/ > > > Cc: [email protected] > > ... > > > @@ -1359,6 +1359,11 @@ int __secure_computing(const struct seccomp_data > > > *sd) > > > this_syscall = sd ? sd->nr : > > > syscall_get_nr(current, current_pt_regs()); > > > > > > +#ifdef CONFIG_X86_64 > > > + if (unlikely(this_syscall == __NR_uretprobe) && !in_ia32_syscall()) > > > + return 0; > > > +#endif > > > > Acked-by: Oleg Nesterov <[email protected]> > > > > > > A note for the seccomp maintainers... > > > > I don't know what do you think, but I agree in advance that the very fact > > this > > patch adds "#ifdef CONFIG_X86_64" into __secure_computing() doesn't look > > nice. > > > > Indeed. in_ia32_syscall() depends arch/x86 too. > We can add an inline function like; > > ``` uprobes.h > static inline bool is_uprobe_syscall(int syscall) > { > // arch_is_uprobe_syscall check can be replaced by Kconfig, > // something like CONFIG_ARCH_URETPROBE_SYSCALL. > #ifdef arch_is_uprobe_syscall > return arch_is_uprobe_syscall(syscall) > #else > return false; > #endif > } > ``` > and > ``` arch/x86/include/asm/uprobes.h > #define arch_is_uprobe_syscall(syscall) \ > (IS_ENABLED(CONFIG_X86_64) && syscall == __NR_uretprobe && > !in_ia32_syscall()) > ```
Notice it'll need to be enclosed in ifdef CONFIG_X86_64 as __NR_uretprobe isn't defined otherwise so the IS_ENABLED isn't needed. > > > The problem is that we need a simple patch for -stable which fixes the real > > problem. We can cleanup this logic later, I think. > > Hmm, at least we should make it is_uprobe_syscall() in uprobes.h so that > do not pollute the seccomp subsystem with #ifdef. I like this approach. Notice it does add a couple of includes that weren't there before: - arch/x86/include/asm/uprobes.h would include asm/unistd.h - seccomp.c would include linux/uprobes.h So it's a less "trivial" patch... If that's ok I can repost with these changes. Eyal.
