Re: [RFC PATCH v2 02/11] ptwrite uprobes: Add minimal low level support for x86

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] x86/uprobes: Uninitialized variable first_phase_done causes invalid fallback
- [High] x86/uprobes: Missing __GFP_ACCOUNT bypasses memcg and GFP_KERNEL deadlocks
- [Medium] x86/uprobes: GFP_HIGHUSER allocation pins unmigratable pages in movable zones
--

commit 692de39e6024ec893235089efa2253699ad076bf
Author: Andi Kleen [off-list ref]

ptwrite uprobes: Add minimal low level support for x86

Add more data structures and the x86 machinery to generate the PTWRITE
instructions for a ptwrite uprobe. The probe executes PTWRITEs and then
jumps back to the original code. In this variant only patching
5 byte nops is supported.

[ ... ]

[ ... ]

+int uprobe_ptwrite_dup_mmap(struct mm_struct *oldmm, struct mm_struct *newmm)
+{
+	struct uprobes_state *old_state = &oldmm->uprobes_state;
+	struct uprobes_state *new_state = &newmm->uprobes_state;
+	struct uprobe_ptwrite_page *ptw, *new;
+	struct vm_area_struct *vma;
+	struct hlist_node *n;
+
+	mmap_assert_write_locked(oldmm);
+	mmap_assert_write_locked(newmm);
+	hlist_for_each_entry(ptw, &old_state->head_ptwrite, node) {
+		void *src, *dst;
+
+		/*
+		 * Not using __GFP_ACCOUNT here because it triggered
+		 * a deadlock.
+		 */
+		new = kzalloc_obj(*new, GFP_KERNEL);
+		if (!new)
+			goto fail;
+		new->page = alloc_page(GFP_KERNEL | __GFP_ZERO);

[Severity: High]
Does stripping __GFP_ACCOUNT here allow containers to bypass memory cgroup
limits? By repeatedly forking processes with active ptwrite uprobes, a local
user could potentially consume unbounded kernel memory without being charged.

Also, since this is called under the mmap_write_lock of both the old and new
mm_structs, is it safe to use GFP_KERNEL allocations? GFP_KERNEL can enter
direct reclaim, which might attempt to reacquire the same or other locks via
page writeback or swap-out, leading to deadlocks.

[ ... ]

[ ... ]

+static struct uprobe_ptwrite_page *
+create_uprobe_ptwrite_page(struct mm_struct *mm, unsigned long vaddr)
+{
+	struct uprobe_ptwrite_page *ptw;
+	struct vm_area_struct *vma;
+	unsigned long area;
+
+	area = find_ptwrite_page_area(mm, vaddr);
+	if (IS_ERR_VALUE(area))
+		return NULL;
+
+	mmap_assert_write_locked(mm);
+
+	ptw = kzalloc_obj(*ptw, GFP_KERNEL);
+	if (!ptw)
+		return NULL;
+
+	ptw->page = alloc_page(GFP_HIGHUSER | __GFP_ZERO);

[Severity: Medium]
Will this allocation pin an unmigratable page in a movable zone?

The GFP_HIGHUSER mask includes the __GFP_MOVABLE flag, but the returned page
is a raw page mapped via _install_special_mapping(). Since it lacks movable_ops
or LRU linkage, it is fundamentally unmigratable. This breaks the page
allocator's mobility contract and can cause permanent memory fragmentation.

[ ... ]

+static int ptwrite_text_poke(struct arch_uprobe *auprobe,
+			     struct vm_area_struct *vma, unsigned long vaddr,
+			     unsigned long stub_addr)
+{
+	u8 jmp5[5] = { 0xe9, 0, 0, 0, 0 };
+	bool first_phase_done;
+	s32 rel;
+	int err;
+
+	if (!ptwrite_rel32(vaddr + 5, stub_addr, &rel))
+		return -ERANGE;
+	memcpy(jmp5 + 1, &rel, 4);
+
+	err = text_poke_5byte(auprobe, vma, vaddr, jmp5,
+			      auprobe->ptwrite.orig[0], false, true, true,
+			      false, &first_phase_done);
+	if (err && first_phase_done) {

[Severity: High]
Can this evaluate uninitialized memory on the error path?

If text_poke_5byte() fails during its first uprobe_write() call (when setting
the INT3 breakpoint), it returns the error immediately without initializing the
first_phase_done out-parameter:

arch/x86/kernel/uprobes.c:text_poke_5byte() {
    ...
	if (!skip_int3) {
		err = uprobe_write(auprobe, vma, vaddr, &int3, 1, verify_insn,
				   is_register, false, &ctx);
		if (err)
			return err;
	}
	if (first_phase_done)
		*first_phase_done = true;
    ...
}

If the uninitialized first_phase_done variable happens to be non-zero (true),
ptwrite_text_poke() will attempt a rollback by calling text_poke_5byte() again
to remove an INT3 breakpoint that was never successfully installed, which could
further fail or corrupt the instruction stream.

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