Re: [PATCH bpf-next v2 05/13] bpf: Account R2 of register-pair returns in live register analysis

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The verifier fails to check if BPF_REG_2 is initialized at BPF_EXIT for global subprograms returning > 8 bytes, which can lead to leaking uninitialized memory.
- [High] The verifier fails to copy the callee's R2 to the caller's R2 in prepare_func_exit() for static subprograms returning > 8 bytes.
--

--- Patch [5]: [PATCH bpf-next v2 05/13] bpf: Account R2 of register-pair returns in live register analysis ---
commit 149c9afee6775ff575a63e08693b340337d96491
Author: Yonghong Song <[email protected]>

bpf: Account R2 of register-pair returns in live register analysis

This commit updates the live register analysis in the BPF verifier to mark R2
as read at BPF_EXIT for subprograms returning a value larger than 8 bytes. It
uses bpf_ret_reg_pair() to determine if a subprogram returns its value in the
R0:R2 register pair.

> diff --git a/kernel/bpf/liveness.c b/kernel/bpf/liveness.c
> index 0aadfbae0acc5..68df6d8db4bda 100644
> --- a/kernel/bpf/liveness.c
> +++ b/kernel/bpf/liveness.c
[ ... ]
> @@ -2174,7 +2179,7 @@ static void compute_insn_live_regs(struct bpf_verifier_env *env,
>  			break;
>  		case BPF_EXIT:
>  			def = 0;
> -			use = r0;
> +			use = ret_reg_pair ? (r0 | BIT(BPF_REG_2)) : r0;

[Severity: High]
Does check_global_subprog_return_code() also need to check BPF_REG_2 when the
subprogram returns a register pair?

If a global BPF subprogram returns a 16-byte value (e.g., an __int128 or
struct) but does not write to R2 before exiting, the verifier only validates
that BPF_REG_0 is initialized:

kernel/bpf/verifier.c:check_global_subprog_return_code() {
    ...
	return check_global_ret_scalar_reg(env, BPF_REG_0);
}

Although check_global_ret_scalar_reg() was refactored earlier in this series
so that it can be reused for R2, there appears to be no call added to check R2
in check_global_subprog_return_code().

Can this allow a global subprogram to leak uninitialized kernel memory or
pointers in the CPU register corresponding to R2 back to the kernel caller?

[Severity: High]
Additionally, should prepare_func_exit() copy the callee's R2 to the caller's
R2 for static subprograms returning > 8 bytes?

Currently, prepare_func_exit() only copies R0:

kernel/bpf/verifier.c:prepare_func_exit() {
    ...
	} else {
		/* return to the caller whatever r0 had in the callee */
		caller->regs[BPF_REG_0] = *r0;
	}
    ...
}

If a BPF program calls a static subprogram that returns a 16-byte value, and
the caller attempts to use the upper 8 bytes returned in R2, won't the verifier
reject the valid program with a read of uninitialized register error? 

This happens because the caller's R2 was marked NOT_INIT prior to the call by
clear_caller_saved_regs(), but prepare_func_exit() does not propagate the
callee's R2 value back to the caller.

>  			break;
>  		case BPF_CALL:
>  			def = ALL_CALLER_SAVED_REGS;

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