Re: [RFC v1 10/19] ptwrite uprobes: Minimal memory references and fault handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Premature publication of `ptw->nblocks` without memory barriers exposes uninitialized memory to concurrent page fault handlers.
- [Medium] Rolling back `ptw->nblocks` on error allows in-place reuse of array slots without an RCU grace period, causing torn reads in concurrent fault handlers.
--

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

ptwrite uprobes: Minimal memory references and fault handling

Add support for memory references. Currently this is only simple cases,
no indirect memory references or strings. This requires fault handling
using the fault notifier hook added earlier.

> diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c
> index df652c56414b5..90e702a4a8e99 100644
> --- a/arch/x86/kernel/uprobes.c
> +++ b/arch/x86/kernel/uprobes.c
[ ... ]
> @@ -1689,13 +1852,22 @@ int arch_uprobe_install_ptwrite(struct arch_uprobe *auprobe,
>  		return -ENOMEM;
>  
>  	block_off = ptw->cursor;
> -	if (block_off > PAGE_SIZE ||
> -	    ptw_a->stub_len > PAGE_SIZE - block_off)
> +	if (block_off > PAGE_SIZE || ptw_a->stub_len > PAGE_SIZE - block_off)
> +		return -ENOMEM;
> +	if (ptw->nblocks >= ARRAY_SIZE(ptw->index))
>  		return -ENOMEM;
>  	stub_addr = ptw->vaddr + block_off;
>  	if (!ptwrite_rel32(stub_addr + ptw_a->jmp_off + 4,
>  			   vaddr + 5, &rel))
>  		return -ERANGE;
> +	site_delta = (s64)vaddr - (s64)ptw->vaddr;
> +	if (site_delta < INT_MIN || site_delta > INT_MAX)
> +		return -ERANGE;
> +
> +	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->nblocks++;
>  
>  	kaddr = kmap_local_page(ptw->page);
>  	memcpy(kaddr + block_off, ptw_a->stub, ptw_a->stub_len);

[Severity: Medium]
Does this code expose uninitialized memory to concurrent page fault handlers?

Since ptw->nblocks is incremented before memcpy() initializes the stub
block memory, a concurrent lockless reader in uprobe_ptwrite_handle_fault()
might observe the incremented nblocks and read the uninitialized fault table
while memcpy() is actively writing to it.

> @@ -1704,9 +1876,13 @@ int arch_uprobe_install_ptwrite(struct arch_uprobe *auprobe,
>  	kunmap_local(kaddr);
>  
>  	ret = ptwrite_text_poke(auprobe, vma, vaddr, stub_addr);
> -	if (!ret)
> -		ptw->cursor = block_off + ptw_a->stub_len;
> -	return ret;
> +	if (ret)
> +		/* Publish rollback before readers use the reduced block count. */
> +		smp_store_release(&ptw->nblocks, ptw->nblocks - 1);
> +		return ret;
> +	}

[Severity: Medium]
Can this rollback of ptw->nblocks cause use-after-free or torn reads for
concurrent readers?

If ptwrite_text_poke() fails, ptw->nblocks is rolled back. However, a
lockless reader executing uprobe_ptwrite_handle_fault() under rcu_read_lock()
might have already read the old ptw->nblocks and be actively iterating over
this slot. A subsequent probe installation could then immediately reuse this
array slot and block_off memory without waiting for a synchronize_rcu()
grace period, causing the lockless reader to observe torn data.

> +	ptw->cursor = block_off + ptw_a->stub_len;
> +	return 0;
>  }

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