Re: [PATCH bpf-next v2 3/5] bpf: alloc_prog_pack(): skip ROX management for already ROX memory

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] BPF trampolines and non-pack JIT binary allocators will crash due to writing directly to ROX memory when EXECMEM_ROX_CACHE is enabled.
--

--- Patch [3]: [PATCH bpf-next v2 3/5] bpf: alloc_prog_pack(): skip ROX management for already ROX memory ---
commit 81282c502cc862cd297ba34160f028b0565526c2
Author: Mike Rapoport (Microsoft) <rppt@kernel.org>

bpf: alloc_prog_pack(): skip ROX management for already ROX memory

execmem_alloc() can return ROX memory that is already filled with
architecture defined trapping instructions.

In preparation for enabling this mode for BPF on x86, make sure that there
is no redundant management of the ROX memory.

There is no need to fill allocated memory with trapping instructions, to
request permissions reset on free and to set ROX permissions as this all
is handled by execmem_alloc().

Add bpf_jit_mem_is_rox() wrapper for execmem_is_rox(), use it to check if
execmem_alloc() returns ROX memory and skip the redundant steps in that
case.

> diff --git a/kernel/bpf/core.c b/kernel/bpf/core.c
> index fc75625dc9513..1b89c18cf2461 100644
> --- a/kernel/bpf/core.c
> +++ b/kernel/bpf/core.c
> @@ -916,6 +916,11 @@ static LIST_HEAD(pack_list);
>  
>  #define BPF_PROG_CHUNK_COUNT (BPF_PROG_PACK_SIZE / BPF_PROG_CHUNK_SIZE)
>  
> +static bool bpf_jit_mem_is_rox(void)
> +{
> +	return execmem_is_rox(EXECMEM_BPF);
> +}
> +

[ ... ]

> @@ -965,7 +972,7 @@ void *bpf_prog_pack_alloc(u32 size, bpf_jit_fill_hole_t bpf_fill_ill_insns, bool
>  			pr_warn_once("BPF: Predictors not flushed for allocations greater than BPF_PROG_PACK_SIZE\n");
>  		size = round_up(size, PAGE_SIZE);
>  		ptr = bpf_jit_alloc_exec(size);
> -		if (ptr) {
> +		if (ptr && !bpf_jit_mem_is_rox()) {
>  			int err;
>  
>  			bpf_fill_ill_insns(ptr, size);

[Severity: Medium]
Since this patch protects bpf_prog_pack_alloc() and alloc_new_pack() from
writing to memory that might already be ROX, does it also need to protect
other callers of bpf_jit_alloc_exec() that write directly to the allocated
memory?

For example, bpf_jit_binary_alloc() allocates memory and directly writes
illegal instructions:

kernel/bpf/core.c:bpf_jit_binary_alloc() {
    ...
    hdr = bpf_jit_alloc_exec(size);
    if (!hdr) {
        bpf_jit_uncharge_modmem(size);
        return NULL;
    }

    /* Fill space with illegal/arch-dep instructions. */
    bpf_fill_ill_insns(hdr, size);
    ...
}

Similarly, on x86, arch_bpf_trampoline_size() allocates memory and passes
it as a read-write buffer:

arch/x86/net/bpf_jit_comp.c:arch_bpf_trampoline_size() {
    ...
    image = bpf_jit_alloc_exec(PAGE_SIZE);
    if (!image)
        return -ENOMEM;

    ret = __arch_prepare_bpf_trampoline(&im, image, image + PAGE_SIZE, image,
                                        m, flags, tnodes, func_addr);
    ...
}

Could these paths cause a page fault when EXECMEM_ROX_CACHE is enabled,
since they attempt to write directly to the read-only memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-execmem-x86-rox-bpf-v0-v2-0-bfd956d35119@kernel.org?part=3