Re: [RFC bpf-next 4/9] bpf: Add kfuncs to allocate and seal user pages

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf: broken icache flush ordering and sync in bpf_user_page_exec()
- [Medium] bpf: READ_IMPLIES_EXEC cleared too late in bpf_user_page_alloc()
- [Medium] bpf: signed long return in bpf_user_page_alloc() is negative for 32-bit user addresses
- [Low] bpf: link failure on !CONFIG_MMU in bpf_user_page_exec()
--

commit bb8df072f0f4bad453d939bfbd2cb461ada4757f
Author: Jiri Olsa <jolsa@kernel.org>

bpf: Add kfuncs to allocate and seal user pages

This commit adds the sleepable bpf_user_page_alloc() and
bpf_user_page_exec() kfuncs for kprobe/uprobe BPF programs to allocate
anonymous user pages in the current process and seal individual pages as
read/execute.

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 4d9e9423826a9..246f4969a144a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c

[ ... ]

> @@ -3739,6 +3743,127 @@ __bpf_kfunc int bpf_copy_from_user_mm(void *dst, u32 dst__sz,
>  	return ret;
>  }
>  
> +/* vm_private_data token for the unmergeable anonymous mapping below. */
> +static char bpf_user_page_marker;
> +
> +/**
> + * bpf_user_page_alloc() - Allocate lazy anonymous pages in the current mm
> + * @nr_pages: Number of pages to allocate
> + *
> + * Creates a read/write anonymous mapping in the current task's address space.
> + * The mapping is kept non-executable even for tasks with READ_IMPLIES_EXEC,
> + * to avoid creating a writable executable mapping. The physical page is
> + * allocated on its first access. The mapping remains until userspace unmaps
> + * it or the process exits.
> + *
> + * Return: The userspace address on success, or a negative error code.
> + */
> +__bpf_kfunc long bpf_user_page_alloc(unsigned long nr_pages)
> +{
> +	const unsigned long prot = PROT_READ | PROT_WRITE;
> +	const unsigned long flags = MAP_PRIVATE | MAP_ANONYMOUS;
> +	struct mm_struct *mm = current->mm;
> +	unsigned long addr, populate, len;
> +	struct vm_area_struct *vma;
> +	int err;
> +
> +	if (unlikely(in_interrupt() || current->flags & (PF_KTHREAD | PF_EXITING)))
> +		return -EPERM;
> +	if (unlikely(!nmi_uaccess_okay()))
> +		return -EPERM;
> +	if (unlikely(!mm))
> +		return -EFAULT;
> +	if (unlikely(!nr_pages || nr_pages > (ULONG_MAX >> PAGE_SHIFT)))
> +		return -EINVAL;
> +
> +	len = nr_pages << PAGE_SHIFT;
> +	err = security_mmap_file(NULL, prot, flags);
> +	if (err)
> +		return err;
> +
> +	if (mmap_write_lock_killable(mm))
> +		return -EINTR;
> +
> +	addr = do_mmap(NULL, 0, len, prot, flags,
> +		       mk_vma_flags(VMA_DONTEXPAND_BIT, VMA_DONTCOPY_BIT, VMA_DONTDUMP_BIT),
> +		       0, &populate, NULL);
> +	if (!IS_ERR_VALUE(addr)) {
> +		/* VM_DONTEXPAND prevents merging this mapping into another VMA. */
> +		vma = find_vma(mm, addr);
> +		vma_start_write(vma);
> +		if (current->personality & READ_IMPLIES_EXEC) {
> +			vm_flags_clear(vma, VM_EXEC);
> +			vma_set_page_prot(vma);
> +		}
> +		vma->vm_private_data = &bpf_user_page_marker;
> +	}

[Severity: Medium]
When current->personality has READ_IMPLIES_EXEC set, is clearing VM_EXEC
after do_mmap() too late?

Before bpf_user_page_alloc() clears VM_EXEC on the VMA, both
security_mmap_file() and do_mmap() evaluate the request with PROT_EXEC
implied:

security/security.c:mmap_prot()
    if (!(current->personality & READ_IMPLIES_EXEC))
        return prot;
    if (!file)
        return prot | PROT_EXEC;

mm/mmap.c:do_mmap()
    if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
        if (!(file && path_noexec(&file->f_path)))
            prot |= PROT_EXEC;
    ...
    mmap_region()
        if (map_deny_write_exec(&vma_flags, &vma_flags))
            return -EACCES;

Because prot becomes PROT_READ | PROT_WRITE | PROT_EXEC during those calls,
selinux_mmap_file() checks PROCESS__EXECMEM for an anonymous W+X mapping and
map_deny_write_exec() rejects the mapping with -EACCES when MMF_HAS_MDWE is
enabled, and perf_event_mmap() emits an executable PERF_RECORD_MMAP event
before VM_EXEC is cleared.

> +
> +	mmap_write_unlock(mm);
> +
> +	if (!IS_ERR_VALUE(addr) && populate)
> +		mm_populate(addr, populate);
> +	return addr;
> +}

[Severity: Medium]
On 32-bit architectures (such as x86-32), can returning addr as a signed
long in bpf_user_page_alloc() turn valid userspace addresses into negative
values?

do_mmap() returns an unsigned long where valid 32-bit user addresses in
[0x80000000, 0xfffff000) have bit 31 set and pass !IS_ERR_VALUE(addr), but
returning addr as long makes those addresses negative and indistinguishable
from error codes.

In addition, long is 64 bits wide in BPF target programs while long in a
32-bit kernel's vmlinux BTF has ret_size = 4, so emit_kfunc_call() in
arch/x86/net/bpf_jit_comp32.c only populates the low 32 bits of R0.

Should bpf_user_page_alloc() return s64 instead, for example returning
(long)addr when IS_ERR_VALUE(addr) is true and (s64)addr otherwise?

> +
> +/**
> + * bpf_user_page_exec() - Make a BPF-allocated user page read/execute
> + * @addr: Address of the page to change
> + *
> + * Changes a writable page created by bpf_user_page_alloc() to read/execute.
> + * The page cannot be made writable again after this call.
> + *
> + * Return: 0 on success, or a negative error code.
> + */
> +__bpf_kfunc int bpf_user_page_exec(unsigned long addr)
> +{
> +	struct mm_struct *mm = current->mm;
> +	struct vm_area_struct *vma;
> +	struct page *page;
> +	int err = -EINVAL;

[ ... ]

> +	vma_start_write(vma);
> +
> +	err = get_user_pages(addr, 1, FOLL_WRITE, &page);
> +	if (err != 1) {
> +		if (!err)
> +			err = -EFAULT;
> +		goto out;
> +	}
> +
> +	flush_icache_user_page(vma, page, addr, PAGE_SIZE);

[Severity: High]
Does calling flush_icache_user_page() in bpf_user_page_exec() before
do_mprotect_pkey_locked() leave stale instructions in the instruction cache?

At the point where flush_icache_user_page() is called, the VMA still has
VM_READ | VM_WRITE without VM_EXEC. Several architectures gate
flush_icache_user_page() on VM_EXEC and turn this call into a no-op:

arch/riscv/include/asm/cacheflush.h:
    #define flush_icache_user_page(vma, pg, addr, len) \
    do {                                               \
        if (vma->vm_flags & VM_EXEC)                   \
            flush_icache_mm(vma->vm_mm, 0);            \
    } while (0)

arch/openrisc/include/asm/cacheflush.h:
    #define flush_icache_user_page(vma, page, addr, len) \
    do {                                                 \
        if (vma->vm_flags & VM_EXEC)                     \
            sync_icache_dcache(page);                    \
    } while (0)

arch/alpha/kernel/smp.c:flush_icache_user_page()
    if ((vma->vm_flags & VM_EXEC) == 0)
        return;

In addition:

1. Because flush_icache_user_page() runs while the PTE is still writable and
before do_mprotect_pkey_locked() write-protects the PTE and flushes remote
TLBs, another thread in current->mm can write to addr after the icache flush
and before write access is removed.

2. On arm64 and riscv, mprotect's set_ptes() path only synchronizes caches
when PG_dcache_clean is clear on the folio and then sets PG_dcache_clean
(for example in arch/arm64/mm/flush.c:__sync_icache_dcache()). If a
multi-page allocation from bpf_user_page_alloc() is backed by a multi-page
anonymous folio (mTHP), sealing a second modified subpage of the same folio
will see PG_dcache_clean already set and skip cache maintenance unless
flush_dcache_page(page) is called to clear PG_dcache_clean.

3. On LoongArch (arch/loongarch/include/asm/cacheflush.h), both
flush_icache_user_page() and flush_dcache_page() are no-ops and set_pte()
does not emit an instruction barrier.

> +	err = do_mprotect_pkey_locked(addr, PAGE_SIZE, PROT_READ | PROT_EXEC, -1);

[Severity: Low]
Will calling do_mprotect_pkey_locked() unconditionally in
bpf_user_page_exec() fail to link when CONFIG_BPF_SYSCALL=y and
CONFIG_MMU=n?

kernel/bpf/helpers.c is built whenever CONFIG_BPF_SYSCALL=y, which does not
depend on CONFIG_MMU, whereas do_mprotect_pkey_locked() is only defined in
mm/mprotect.c when CONFIG_MMU=y:

mm/Makefile:
    mmu-$(CONFIG_MMU) := highmem.o memory.o mincore.o \
                         mlock.o mmap.o mmu_gather.o mprotect.o mremap.o \

include/linux/mm.h declares do_mprotect_pkey_locked() without a !CONFIG_MMU
stub, and mm/nommu.c:do_mmap() allocates non-lazy physical memory that
cannot be transitioned to read/execute via page tables.

> +	if (!err) {
> +		vma = find_vma(mm, addr);
> +		vm_flags_clear(vma, VM_MAYWRITE);
> +	}
> +	put_page(page);
> +out:
> +	mmap_write_unlock(mm);
> +	return err;
> +}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261021195759.119421-1-jolsa@kernel.org?part=4