Re: [PATCH bpf-next v2 1/6] bpf: Keep fault protection when merging pointer types

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

Pre-existing issues:
- [Critical] The verifier fails to emit BPF_PROBE_MEM instructions when a safe pointer type (e.g., PTR_TO_STACK or PTR_TO_MAP_VALUE) and an untrusted memory pointer type (PTR_TO_MEM | PTR_UNTRUSTED) reach the same load instruction via different branches. This completely bypasses the newly introduced merge_ptr_types logic.
--

--- Patch [1]: [PATCH bpf-next v2 1/6] bpf: Keep fault protection when merging pointer types ---
commit f6edd0237d3f5f1dd3f7b1e12e20100d5ce150cd
Author: Daniel Borkmann <[email protected]>

bpf: Keep fault protection when merging pointer types

This commit fixes an issue where merging different pointer types in
save_aux_ptr_type() drops flags like PTR_UNTRUSTED. It introduces
merge_ptr_types() to properly normalize and combine pointer types so that the
BPF_PROBE_MEM rewrite remains intact.

> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 6ac1afced20bf..ad6ccd5cf51db 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c

[ ... ]

> @@ -17092,20 +17097,12 @@ static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type typ
>  		    is_ptr_to_mem_or_btf_id(*prev_type)) {
>  			/*
>  			 * Have to support a use case when one path through
> -			 * the program yields TRUSTED pointer while another
> -			 * is UNTRUSTED. Fallback to UNTRUSTED to generate
> -			 * BPF_PROBE_MEM/BPF_PROBE_MEMSX.
> -			 * Same behavior of MEM_RDONLY flag.
> +			 * the program yields a TRUSTED pointer while another
> +			 * is UNTRUSTED. Merge them into a type which keeps
> +			 * the BPF_PROBE_MEM/BPF_PROBE_MEMSX rewrite when
> +			 * either side needs it.
>  			 */
> -			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;
> -			*prev_type = merged_type;
> +			*prev_type = merge_ptr_types(type, *prev_type);

[Severity: Critical]
This is a pre-existing issue, but does this bypass conflict resolution
entirely if reg_type_mismatch() returns false for both types?

If one branch provides a safe pointer (like PTR_TO_STACK) and another branch
provides an untrusted pointer (PTR_TO_MEM | PTR_UNTRUSTED), they evaluate as:

kernel/bpf/verifier.c:reg_type_mismatch() {
	return src != prev && (!reg_type_mismatch_ok(src) ||
			       !reg_type_mismatch_ok(prev));
}

Since neither base type is in the reg_type_mismatch_ok() switch statement,
it returns true for both types, causing reg_type_mismatch() to return false.

If the safe pointer reaches save_aux_ptr_type() first, we wouldn't enter
the else if (reg_type_mismatch(type, *prev_type)) block at all.

Does this mean merge_ptr_types() is skipped, the untrusted pointer type is
silently ignored, and we might fail to emit the required BPF_PROBE_MEM
rewrite?

>  		} else {
>  			verbose(env, "same insn cannot be used with different pointers\n");
>  			return -EINVAL;

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