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

[sashiko-bot@kernel.org]

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?

[ ... ]

> @@ -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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260514135342.22130-1-jolsa@kernel.org?part=1