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 <[email protected]> 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. > diff --git a/arch/x86/kernel/uprobes.c b/arch/x86/kernel/uprobes.c > index 65a2de82ecd29..7fcdc4bf5197a 100644 > --- a/arch/x86/kernel/uprobes.c > +++ b/arch/x86/kernel/uprobes.c [ ... ] > @@ -719,6 +723,134 @@ static struct vm_area_struct > *get_uprobe_trampoline(struct mm_struct *mm, unsign [ ... ] > +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. [ ... ] > @@ -1102,6 +1287,475 @@ static int copy_from_vaddr(struct mm_struct *mm, > unsigned long vaddr, void *dst, [ ... ] > +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/[email protected]?part=2
