Re: [RFC v1 13/19] ptwrite uprobes: Support instruction puning

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stack out-of-bounds read in `pun_install` when copying the original instruction.
- [High] Uninstalling a punned uprobe fails and permanently corrupts process memory if the user modifies the instruction's subsequent bytes.
--

commit bfefc3cb6738d81a0fc8093e66776a8584e4deed
Author: Andi Kleen <ak@kernel.org>

ptwrite uprobes: Support instruction puning

The previous ptwrite instrumentation only worked on 5 byte+ nops because
it needs to patch in a 5 byte branch. But the instruction may be shorter
and the kernel cannot prove that nobody jumps to the next instruction.

[ ... ]

> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index 20557423690f9..806e40f7b0ab3 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
> @@ -1866,10 +2001,210 @@ static int ptwrite_text_poke(struct arch_uprobe *auprobe,
>  	return err;
>  }
>  
> +/*
> + * Replace an aligned five-byte NOP run with a JMP in one eight-byte store.
> + * The trailing three bytes are read from the existing text. We assume
> + * nobody else is changing it. This is covered by the Intel/AMD "aligned store"
> + * cross modifying guarantee.
> + */

[ ... ]

> +static int pun_install(struct arch_uprobe *auprobe,
> +			       struct vm_area_struct *vma, unsigned long vaddr,
> +			       const u8 *orig)
> +{
> +	struct mm_struct *mm = vma->vm_mm;
> +	struct uprobe_ptwrite_page *ptw;
> +	struct uprobe_ptwrite_arch *ptw_a = &auprobe->ptwrite;
> +	struct uprobes_state *state = &mm->uprobes_state;
> +	struct write_opcode_ctx ctx = {
> +		.base = vaddr,
> +		.expect = EXPECT_BYTE,
> +		.expect_byte = orig[0],
> +	};
> +	unsigned long t, page_base, block_off, stub_addr;
> +	s64 site_delta;
> +	s32 jump_rel, disp32, orig_rel;
> +	u8 site_len;
> +	bool found = false;

[ ... ]

> +	/*
> +	 * A NOP fallback needs a synthetic rel32 at the site, so it uses
> +	 * the full five-byte poke and restore path rather than punning.
> +	 */
> +	site_len = nop_fallback ? 5 : ptw_a->len;
> +	ptw->index[ptw->nblocks].off = block_off;
> +	ptw->index[ptw->nblocks].len = ptw_a->stub_len;
> +	ptw->index[ptw->nblocks].ft_off = ptw_a->ft_off;
> +	ptw->index[ptw->nblocks].pun = !nop_fallback;
> +	ptw->index[ptw->nblocks].orig0 = orig[0];
> +	ptw->index[ptw->nblocks].site_len = site_len;
> +	ptw->index[ptw->nblocks].site_off = (s32)site_delta;
> +	memcpy(ptw->index[ptw->nblocks].site_insn, orig, site_len);

[Severity: High]
Does this memcpy() read out-of-bounds? 

The original instruction is passed down via the orig parameter from
arch_uprobe_install_ptwrite(). In arch_uprobe_install_ptwrite(), orig is
declared as a 5-byte array on the stack.

However, site_len here can be up to 15 bytes (MAX_UINSN_BYTES) for generic
instructions when punning is active. If site_len is greater than 5, this will
copy uninitialized kernel stack memory into the permanent site_insn array.

Later, when attempting to reinstall the uprobe, the memcmp() against the true
original instruction will fail due to the garbage bytes. This breaks the
collision check and could cause a trampoline slot leak on every reinstall.

> +	smp_store_release(&ptw->nblocks, ptw->nblocks + 1);

[ ... ]

> @@ -1937,15 +2301,63 @@ int arch_uprobe_uninstall_ptwrite(struct arch_uprobe *auprobe,
>  		struct vm_area_struct *vma, unsigned long vaddr)
>  {
>  	struct mm_struct *mm = vma->vm_mm;
> +	struct uprobe_ptwrite_arch *ptw_a = &auprobe->ptwrite;
> +	struct uprobes_state *state = &mm->uprobes_state;
>  	u8 cur[5];
> +	int b;
> +	struct write_opcode_ctx ctx = {
> +		.base = vaddr,
> +		.expect = EXPECT_BYTE,
> +		.expect_byte = 0xe9,
> +	};
>  
>  	mmap_assert_write_locked(mm);
> -	if (copy_from_vaddr(mm, vaddr, cur, sizeof(cur)) ||
> -	    !ptwrite_is_installed(mm, vaddr, cur))
> -		return;
> +	{
> +		struct uprobe_ptwrite_page *ptw;
> +		struct uprobe_ptwrite_page *fpw = NULL;
> +		s32 rel;
> +		s64 target;
> +		unsigned long page_base, boff;
> +		int ret;
> +
> +		ret = copy_from_vaddr(mm, vaddr, cur, sizeof(cur));
> +		if (ret)
> +			return ret;
> +		if (!ptwrite_is_installed(mm, vaddr, cur))
> +			return 0;

[Severity: High]
Does this validation incorrectly abandon punned uprobes?

If a punned uprobe is installed on an instruction with a relative offset,
bytes 1-4 are mutable user code. The user application (e.g., a JIT compiler
or dynamic linker) might legitimately update the relative offset in these
subsequent bytes.

When the uprobe is uninstalled, ptwrite_is_installed() strictly requires the
JMP target to still point to the trampoline. Since the user modified the
relative displacement, ptwrite_is_installed() returns false.

This assumes the probe was entirely overwritten and aborts the uninstall
prematurely, returning 0. As a result, the injected 0xe9 (JMP) opcode is left
permanently in the user's execution stream, which can corrupt the instruction
stream and cause crashes or erratic behavior.

> +
> +		memcpy(&rel, cur + 1, sizeof(rel));
> +		target = (s64)vaddr + 5 + (s64)rel;

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