Re: [PATCH bpf-next 1/6] bpf: Reject load-acquire from pointers requiring fault protection

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The verifier fails to convert BPF_LDX instructions to BPF_PROBE_MEM when paths with PTR_TO_MEM and PTR_TO_BTF_ID | PTR_UNTRUSTED merge, allowing unhandled page faults.
--

--- Patch [1]: [PATCH bpf-next 1/6] bpf: Reject load-acquire from pointers requiring fault protection ---
commit 7bfb4a68d477eb7f566fc7d106ee4703a225c192
Author: Daniel Borkmann <[email protected]>

bpf: Reject load-acquire from pointers requiring fault protection

This patch prevents BPF_LOAD_ACQ instructions from being used with
pointer types that require exception table entries for fault protection.
It rejects untrusted pointers and bare PTR_TO_BTF_ID pointers since the
JIT does not emit exception tables for load-acquires, which could result
in a kernel panic on a page fault.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index d952bd95cbb7b..78637faf13508 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -4921,6 +4921,30 @@ static bool is_arena_reg(struct bpf_verifier_env *env, int regno)
>  	return reg->type == PTR_TO_ARENA;
>  }
>  
> +static bool is_load_acq_unsafe(struct bpf_verifier_env *env, int regno,
> +			       struct bpf_insn *insn)
> +{
> +	const struct bpf_reg_state *reg = reg_state(env, regno);
> +
> +	/*
> +	 * A BPF_LOAD_ACQ is not rewritten to a BPF_PROBE_MEM load by the
> +	 * verifier, unlike a regular BPF_LDX. The JIT would emit a plain load
> +	 * with no exception table entry, so a fault (e.g. NULL deref) crashes
> +	 * the kernel instead of being handled.
> +	 *
> +	 * Reject the source pointer types that a BPF_LDX would have had that
> +	 * fault protection applied to, i.e. the ones bpf_convert_ctx_accesses()
> +	 * turns into BPF_PROBE_MEM: a bare PTR_TO_BTF_ID and any PTR_UNTRUSTED
> +	 * pointer (untrusted btf ids, untrusted MEM_ALLOC, rdonly untrusted
> +	 * memory). A PTR_TRUSTED pointer is not among them, is not converted,
> +	 * and stays allowed. Same for the other flagged PTR_TO_BTF_ID variants
> +	 * (MEM_ALLOC, MEM_RCU, ...), hence the exact match on the base type.
> +	 */
> +	return insn->imm == BPF_LOAD_ACQ &&
> +	       (reg->type == PTR_TO_BTF_ID ||
> +		(type_flag(reg->type) & PTR_UNTRUSTED));

[Severity: High]
This is a pre-existing issue, but since the comment mentions pointer types
that bpf_convert_ctx_accesses() turns into BPF_PROBE_MEM, does it currently
miss handling PTR_TO_MEM | PTR_UNTRUSTED?

When paths with PTR_TO_MEM and PTR_TO_BTF_ID | PTR_UNTRUSTED merge,
save_aux_ptr_type() produces PTR_TO_MEM | PTR_UNTRUSTED without MEM_RDONLY:

kernel/bpf/verifier.c:save_aux_ptr_type() {
    ...
			if (is_ptr_to_mem(type) || is_ptr_to_mem(*prev_type))
				merged_type = PTR_TO_MEM;
			else
				merged_type = PTR_TO_BTF_ID;
			if ((type & PTR_UNTRUSTED) || (*prev_type & PTR_UNTRUSTED))
				merged_type |= PTR_UNTRUSTED;
			if ((type & MEM_RDONLY) || (*prev_type & MEM_RDONLY))
				merged_type |= MEM_RDONLY;
    ...
}

However, in kernel/bpf/fixups.c:bpf_convert_ctx_accesses(), it seems this
exact type combination is not converted to BPF_PROBE_MEM because the switch
only handles the MEM_RDONLY case:

kernel/bpf/fixups.c:bpf_convert_ctx_accesses() {
    ...
		case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
		case PTR_TO_MEM | MEM_RDONLY | PTR_UNTRUSTED:
			if (type == BPF_READ) {
    ...
}

Could this lead to an unhandled page fault and kernel panic if an untrusted
pointer and a valid writable memory pointer (like from a dynptr) reach the
same BPF_LDX instruction, bypassing the fault protection?

> +}
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1