Re: Crash when attaching uretprobes to processes running in Docker

[Jiri Olsa <olsajiri@gmail.com>]

On Sat, Jan 11, 2025 at 07:40:15PM +0100, Jiri Olsa wrote:
> On Sat, Jan 11, 2025 at 02:25:37AM +1100, Aleksa Sarai wrote:
> > On 2025-01-10, Eyal Birger <eyal.birger@gmail.com> wrote:
> > > Hi,
> > > 
> > > When attaching uretprobes to processes running inside docker, the attached
> > > process is segfaulted when encountering the retprobe. The offending commit
> > > is:
> > > 
> > > ff474a78cef5 ("uprobe: Add uretprobe syscall to speed up return probe")
> > > 
> > > To my understanding, 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.
> > 
> > FWIW, the default seccomp profile of Docker _should_ return -ENOSYS for
> > uretprobe (runc has a bunch of ugly logic to try to guarantee this if
> > Docker hasn't updated their profile to include it). Though I guess that
> > isn't sufficient for the magic that uretprobe(2) does...
> > 
> > > This behavior can be reproduced by the below bash script, which works before
> > > this commit.
> > > 
> > > Reported-by: Rafael Buchbinder <rafi@rbk.io>
> 
> hi,
> nice ;-) thanks for the report, the problem seems to be that uretprobe syscall
> is blocked and uretprobe trampoline does not expect that
> 
> I think we could add code to the uretprobe trampoline to detect this and
> execute standard int3 as fallback to process uretprobe, I'm checking on that

hack below seems to fix the issue, it's using rbx to signal that uretprobe
syscall got executed, if not, trampoline does int3 and executes uretprobe
handler in the old way

unfortunately now the uretprobe trampoline size crosses the xol slot limit so
will need to come up with some generic/arch code solution for that, code below
is neglecting that for now

jirka


---
 arch/x86/kernel/uprobes.c | 24 ++++++++++++++++++++++++
 include/linux/uprobes.h   |  1 +
 kernel/events/uprobes.c   | 10 ++++++++--
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
index 5a952c5ea66b..b54863f6fa25 100644
--- a/arch/x86/kernel/uprobes.c
+++ b/arch/x86/kernel/uprobes.c
@@ -315,14 +315,25 @@ asm (
 	".global uretprobe_trampoline_entry\n"
 	"uretprobe_trampoline_entry:\n"
 	"pushq %rax\n"
+	"pushq %rbx\n"
 	"pushq %rcx\n"
 	"pushq %r11\n"
+	"movq $1, %rbx\n"
 	"movq $" __stringify(__NR_uretprobe) ", %rax\n"
 	"syscall\n"
 	".global uretprobe_syscall_check\n"
 	"uretprobe_syscall_check:\n"
+	"or %rbx,%rbx\n"
+	"jz uretprobe_syscall_return\n"
 	"popq %r11\n"
 	"popq %rcx\n"
+	"popq %rbx\n"
+	"popq %rax\n"
+	"int3\n"
+	"uretprobe_syscall_return:\n"
+	"popq %r11\n"
+	"popq %rcx\n"
+	"popq %rbx\n"
 
 	/* The uretprobe syscall replaces stored %rax value with final
 	 * return address, so we don't restore %rax in here and just
@@ -338,6 +349,16 @@ extern u8 uretprobe_trampoline_entry[];
 extern u8 uretprobe_trampoline_end[];
 extern u8 uretprobe_syscall_check[];
 
+#define UINSNS_PER_PAGE                 (PAGE_SIZE/UPROBE_XOL_SLOT_BYTES)
+
+bool arch_is_uretprobe_trampoline(unsigned long vaddr)
+{
+	unsigned long start = uprobe_get_trampoline_vaddr();
+	unsigned long end = start + 2*UINSNS_PER_PAGE;
+
+	return vaddr >= start && vaddr < end;
+}
+
 void *arch_uprobe_trampoline(unsigned long *psize)
 {
 	static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
@@ -418,6 +439,9 @@ SYSCALL_DEFINE0(uretprobe)
 	regs->r11 = regs->flags;
 	regs->cx  = regs->ip;
 
+	/* zero rbx to signal trampoline that uretprobe syscall was executed */
+	regs->bx  = 0;
+
 	return regs->ax;
 
 sigill:
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index e0a4c2082245..dbde57a68a1b 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -213,6 +213,7 @@ extern void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
 extern void uprobe_handle_trampoline(struct pt_regs *regs);
 extern void *arch_uprobe_trampoline(unsigned long *psize);
 extern unsigned long uprobe_get_trampoline_vaddr(void);
+bool arch_is_uretprobe_trampoline(unsigned long vaddr);
 #else /* !CONFIG_UPROBES */
 struct uprobes_state {
 };
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index fa04b14a7d72..73df64109f38 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -1703,6 +1703,11 @@ void * __weak arch_uprobe_trampoline(unsigned long *psize)
 	return &insn;
 }
 
+bool __weak arch_is_uretprobe_trampoline(unsigned long vaddr)
+{
+	return vaddr == uprobe_get_trampoline_vaddr();
+}
+
 static struct xol_area *__create_xol_area(unsigned long vaddr)
 {
 	struct mm_struct *mm = current->mm;
@@ -1725,8 +1730,9 @@ static struct xol_area *__create_xol_area(unsigned long vaddr)
 
 	area->vaddr = vaddr;
 	init_waitqueue_head(&area->wq);
-	/* Reserve the 1st slot for get_trampoline_vaddr() */
+	/* Reserve the first two slots for get_trampoline_vaddr() */
 	set_bit(0, area->bitmap);
+	set_bit(1, area->bitmap);
 	insns = arch_uprobe_trampoline(&insns_size);
 	arch_uprobe_copy_ixol(area->page, 0, insns, insns_size);
 
@@ -2536,7 +2542,7 @@ static void handle_swbp(struct pt_regs *regs)
 	int is_swbp;
 
 	bp_vaddr = uprobe_get_swbp_addr(regs);
-	if (bp_vaddr == uprobe_get_trampoline_vaddr())
+	if (arch_is_uretprobe_trampoline(bp_vaddr))
 		return uprobe_handle_trampoline(regs);
 
 	rcu_read_lock_trace();
-- 
2.47.1