Re: [PATCH 1/7] uprobes/x86: Move optimized uprobe from nop5 to nop10

[Jiri Olsa <olsajiri@gmail.com>]

On Thu, May 14, 2026 at 08:05:45PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] set_orig_insn fails to unoptimize uprobes in foreign processes because __is_optimized relies on __in_uprobe_trampoline, which hardcodes current->mm for VMA lookups, resulting in silent stack pointer corruption.
> - [High] set_orig_insn fails to unoptimize a forked child's uprobe due to breakpoint overwriting the lea instruction, leaving a corrupted instruction that truncates the stack pointer.
> --
> 
> commit ecbc682eb0a0b0a7df1fd339bbdb6beeb8e1fc0a
> Author: Jiri Olsa <jolsa@kernel.org>
> 
> uprobes/x86: Move optimized uprobe from nop5 to nop10
> 
> This commit fixes a redzone clobbering issue with optimized uprobes by
> moving them to use a 10-byte nop instruction, allowing space for a stack
> adjustment before the call.
> 
> Link: https://lore.kernel.org/bpf/20260509003146.976844-1-andrii@kernel.org/
> 
> > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> > --- a/arch/x86/kernel/uprobes.c
> > +++ b/arch/x86/kernel/uprobes.c
> 
> [ ... ]
> 
> > @@ -1049,19 +1077,21 @@ static bool __is_optimized(uprobe_opcode_t *insn, unsigned long vaddr)
> >  	struct __packed __arch_relative_insn {
> >  		u8 op;
> >  		s32 raddr;
> > -	} *call = (struct __arch_relative_insn *) insn;
> > +	} *call = (struct __arch_relative_insn *)(insn + LEA_INSN_SIZE);
> >  
> > -	if (!is_call_insn(insn))
> > +	if (!is_lea_insn(insn))
> > +		return false;
> > +	if (!is_call_insn(insn + LEA_INSN_SIZE))
> >  		return false;
> > -	return __in_uprobe_trampoline(vaddr + 5 + call->raddr);
> > +	return __in_uprobe_trampoline(vaddr + OPT_INSN_SIZE + call->raddr);
> >  }
> 
> Does this sequence risk silent stack pointer corruption during uprobe
> unregistration?
> 
> When a tracer unregisters a uprobe from another process, it runs in the
> tracer's context. However, __is_optimized() calls __in_uprobe_trampoline(),
> which hardcodes current->mm for the VMA lookup:
> 
> arch/x86/kernel/uprobes.c:__in_uprobe_trampoline() {
>         struct vm_area_struct *vma = vma_lookup(current->mm, ip);
>         return vma && vma_is_special_mapping(vma, &tramp_mapping);
> }
> 
> Since current is the tracer, the lookup fails, causing __is_optimized()
> to return false.
> 
> If __is_optimized() returns false, set_orig_insn() bypasses
> swbp_unoptimize() and writes only the first byte of the original
> instruction (e.g., 0x66 for nop10) over the 0x48 (lea) byte.
> 
> This leaves the instruction 66 8d 64 24 80 e8 ... in the tracee's memory,
> which decodes as lea -0x80(%rsp), %sp. Could this truncate the stack
> pointer to 16 bits and cause stack corruption when the tracee subsequently
> pushes or pops?

yes, we need to pass vma->vm_mm to __is_optimized and not use current->mm

> 
> [ ... ]
> 
> > @@ -1095,14 +1125,25 @@ int set_orig_insn(struct arch_uprobe *auprobe, struct vm_area_struct *vma,
> >  		  unsigned long vaddr)
> >  {
> >  	if (test_bit(ARCH_UPROBE_FLAG_CAN_OPTIMIZE, &auprobe->flags)) {
> > -		int ret = is_optimized(vma->vm_mm, vaddr);
> > -		if (ret < 0)
> > +		uprobe_opcode_t insn[OPT_INSN_SIZE];
> > +		int ret;
> > +
> > +		ret = copy_from_vaddr(vma->vm_mm, vaddr, &insn, OPT_INSN_SIZE);
> > +		if (ret)
> >  			return ret;
> > -		if (ret) {
> > +		if (__is_optimized((uprobe_opcode_t *)&insn, vaddr)) {
> >  			ret = swbp_unoptimize(auprobe, vma, vaddr);
> >  			WARN_ON_ONCE(ret);
> >  			return ret;
> >  		}
> > +		/*
> > +		 * We can have re-attached probe on top of jmp8 instruction,
> > +		 * which did not get optimized. We need to restore the jmp8
> > +		 * instruction, instead of the original instruction (nop10).
> > +		 */
> > +		if (is_swbp_insn(&insn[0]) && insn[1] == OPT_JMP8_OFFSET)
> > +			return uprobe_write_opcode(auprobe, vma, vaddr, JMP8_INSN_OPCODE,
> > +						   false /* is_register */);
> >  	}
> >  	return uprobe_write_opcode(auprobe, vma, vaddr, *(uprobe_opcode_t *)&auprobe->insn,
> >  				   false /* is_register */);
> 
> Is there a similar issue for forked children that inherit the optimized
> 10-byte sequence?
> 
> During fork, uprobe_mmap() installs a breakpoint in the child by writing
> 0xCC to the first byte, changing the instruction from 48 8d... to cc 8d...
> 
> If the uprobe is unregistered before the child hits and re-optimizes it,
> __is_optimized() will return false because is_lea_insn() strictly expects
> the first byte to be 0x48:
> 
> arch/x86/kernel/uprobes.c:is_lea_insn() {
>         return !memcmp(insn, lea_rsp, LEA_INSN_SIZE);
> }
> 
> The fallback check for the re-attached probe on top of jmp8 also fails
> because insn[1] is 0x8d, not OPT_JMP8_OFFSET.
> 
> Could set_orig_insn() then fall back to writing just the first byte of
> the original instruction over the 0xcc, again leaving 66 8d 64 24 80 e8 ...
> and silently truncating the child's stack pointer?

nice.. maybe we can skip the install_breakpoint call in uprobe_mmap
for optimized probes.. will check

jirka