Re: [PATCH 4/6] mm/page_owner: add bpf_page_owner_get_nid() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: arbitrary kernel memory read via page_owner_scan stack aliasing
- [High] bpf: bpf_page_owner_get_nid() oops on unmapped struct page
--

commit 86f6fa9f642d356a9518f6b22fec24555b1e1568
Author: Zhen Ni <zhen.ni@easystack.cn>

mm/page_owner: add bpf_page_owner_get_nid() kfunc

This commit extracts the NUMA node lookup from read_page_owner() into a shared
page_owner_get_nid() helper. It then exposes this helper to BPF programs via a
new bpf_page_owner_get_nid() kfunc.

> diff --git a/mm/page_owner.c b/mm/page_owner.c
> index eb420803a434a..43a722678accf 100644
> --- a/mm/page_owner.c
> +++ b/mm/page_owner.c
[ ... ]
> @@ -711,6 +711,19 @@ void __dump_page_owner(const struct page *page)
>  	page_ext_put(page_ext);
>  }
>  
> +/*
> + * NUMA node of @page, bypassing PF_POISONED_CHECK() in page_to_nid() to
> + * avoid VM_BUG_ON on poisoned pages; -1 for poisoned pages.
> + */
> +int page_owner_get_nid(struct page *page)
> +{
> +	memdesc_flags_t page_flags = READ_ONCE(page->flags);

[Severity: High]
Can this trigger a kernel oops on an unmapped struct page?

The KF_SLEEPABLE flag on bpf_iter_page_owner_next() allows the BPF program to
be preempted or to explicitly sleep while holding the scan object.

If memory is hot-removed while the program sleeps, the vmemmap region backing
the struct page pointer inside the scan object could be unmapped.

When the program wakes up and calls bpf_page_owner_get_nid(), this raw
READ_ONCE(page->flags) would dereference the potentially unmapped pointer and
crash the kernel.

Should bpf_page_owner_get_nid() use copy_from_kernel_nofault() or BPF probe
read semantics to safely probe the memory instead?

> +
> +	if (page_flags.f == PAGE_POISON_PATTERN)
> +		return -1;
> +	return memdesc_nid(&page_flags);
> +}
> +

> diff --git a/mm/page_owner_iter.c b/mm/page_owner_iter.c
> index 80693d4174077..907517a291443 100644
> --- a/mm/page_owner_iter.c
> +++ b/mm/page_owner_iter.c
[ ... ]
> @@ -165,4 +165,9 @@ __bpf_kfunc int bpf_page_owner_stack_snprint(depot_stack_handle_t handle,
>  	return stack_depot_snprint(handle, buf, buf_size, 0);
>  }
>  
> +__bpf_kfunc int bpf_page_owner_get_nid(struct page_owner_scan *scan)
> +{
> +	return page_owner_get_nid(scan->page);
> +}

[Severity: High]
Is there a risk of an arbitrary kernel memory read here?

Because the scan object physically resides on the BPF program's stack (as
returned by bpf_iter_page_owner_next()), a BPF program can overwrite the
scan->page pointer with an arbitrary kernel address using normal BPF stack
pointers (PTR_TO_STACK).

If the program then passes the aliased PTR_TO_BTF_ID to
bpf_page_owner_get_nid(), wouldn't this kfunc blindly dereference the forged
pointer, bypassing verifier safety checks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009112308.240769-1-zhen.ni@easystack.cn?part=4