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

On 8/4/26 2:14 PM, [email protected] wrote:
> 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.

We should be okay here. The actually allowing R2 is in later commit
  Add verifier support for 16-byte returns in R0:R2

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