[PATCH bpf-next v5 04/11] bpf: Track R2 of register-pair returns in precision backtracking

[Yonghong Song <yonghong.song@linux.dev>]

Precision backtracking treats only R0 as a return register at a
call/return boundary, so once the verifier starts modeling R2 that way,
marking the second half of such a return precise would trip the
"unexpected regs" checks in backtrack_insn() and reject a valid
program.

Marking the upper half precise, for example by branching on it after a
call to a static subprogram, walks backtracking into the callee and
reaches its BPF_EXIT with R2 still set in the mask. Handle R2 like R0
in boundaries where a call defines the return registers.

R2 differs from R0 in that it is an argument register as well, which
makes two things worth spelling out:

 - R2 is only excused from the BPF_REGMASK_ARGS check where the callee
   or kfunc really does return a pair, as reported by
   bpf_ret_reg_pair()/bpf_kfunc_ret_reg_pair(). Any other call leaves R2
   uninitialized, so a request for its precision cannot be legitimate,
   and clearing it unconditionally would turn that verifier bug from a
   loud -EFAULT into a silently dropped precision request.

 - at BPF_EXIT the return registers are sampled before the callback path
   clears R1-R5. That clear does not touch R0, but it does cover R2, and
   running it first would drop a pair return whenever the instruction
   following the call happens to be one that invokes a callback.

Signed-off-by: Yonghong Song <yonghong.song@linux.dev>
---
 kernel/bpf/backtrack.c | 78 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 65 insertions(+), 13 deletions(-)

diff --git a/kernel/bpf/backtrack.c b/kernel/bpf/backtrack.c
index a2b18a9f1694..0c8e05a7e175 100644
--- a/kernel/bpf/backtrack.c
+++ b/kernel/bpf/backtrack.c
@@ -423,6 +423,16 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 				 */
 				verifier_bug_if(idx + 1 != subseq_idx, env,
 						"extra insn from subprog");
+				/* a global subprog returning more than 8 bytes
+				 * sets R2 as well. R2 is part of the args mask
+				 * checked just below, so clear it here rather
+				 * than next to R0. Only a subprog that does
+				 * return a pair defines R2, so leave the mask
+				 * alone otherwise and let the check below catch
+				 * an R2 that has no business being set.
+				 */
+				if (bpf_ret_reg_pair(env, subprog))
+					bt_clear_reg(bt, BPF_REG_2);
 				/* r1-r5 are invalidated after subprog call,
 				 * so for global func call it shouldn't be set
 				 * anymore
@@ -506,6 +516,17 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 				return -ENOTSUPP;
 			/* regular helper call sets R0 */
 			bt_clear_reg(bt, BPF_REG_0);
+			/* a kfunc returning more than 8 bytes also sets R2.
+			 * R2 is part of the args mask checked just below, so
+			 * clear it here rather than next to R0. The prototype
+			 * lookup is only worth doing when R2 is requested at
+			 * all; any other call leaves R2 uninitialized, so a
+			 * request for it is caught by the check below.
+			 */
+			if (bt_is_reg_set(bt, BPF_REG_2) &&
+			    insn->src_reg == BPF_PSEUDO_KFUNC_CALL &&
+			    bpf_kfunc_ret_reg_pair(env, insn))
+				bt_clear_reg(bt, BPF_REG_2);
 			if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
 				/* if backtracking was looking for registers R1-R5
 				 * they should have been found already.
@@ -520,7 +541,41 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 					return -EFAULT;
 			}
 		} else if (opcode == BPF_EXIT) {
-			bool r0_precise;
+			bool from_subprog_call, r0_precise, r2_precise;
+			struct bpf_insn *call;
+			int subprog;
+
+			/* BPF_EXIT in subprog or callback always returns
+			 * right after the call instruction, so by checking
+			 * whether the instruction at subseq_idx-1 is subprog
+			 * call or not we can distinguish actual exit from
+			 * *subprog* from exit from *callback*. In the former
+			 * case, we need to propagate the precision of the
+			 * return registers, if necessary. In the latter we
+			 * never do that.
+			 */
+			from_subprog_call = subseq_idx - 1 >= 0 &&
+					    bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]);
+
+			/* Sample the return registers before the callback
+			 * handling below clears R1-R5: unlike R0, R2 is an
+			 * argument register as well, so that clear would drop
+			 * a pair return on the floor.
+			 */
+			r0_precise = from_subprog_call && bt_is_reg_set(bt, BPF_REG_0);
+			r2_precise = false;
+			if (from_subprog_call && bt_is_reg_set(bt, BPF_REG_2)) {
+				call = &env->prog->insnsi[subseq_idx - 1];
+				subprog = bpf_find_subprog(env, subseq_idx + call->imm);
+				if (subprog < 0)
+					return -EFAULT;
+				/* Only a callee that does return a pair defines
+				 * R2. Leave the mask alone otherwise, so that
+				 * the check below still catches an R2 that has
+				 * no business being set.
+				 */
+				r2_precise = bpf_ret_reg_pair(env, subprog);
+			}
 
 			/* Backtracking to a nested function call, 'idx' is a part of
 			 * the inner frame 'subseq_idx' is a part of the outer frame.
@@ -533,30 +588,27 @@ static int backtrack_insn(struct bpf_verifier_env *env, int idx, int subseq_idx,
 			if (subseq_idx >= 0 && bpf_calls_callback(env, subseq_idx))
 				for (i = BPF_REG_1; i <= BPF_REG_5; i++)
 					bt_clear_reg(bt, i);
+
+			/* a callee returning more than 8 bytes sets R2 as well;
+			 * R2 is part of the args mask checked just below, so
+			 * clear it here rather than next to R0.
+			 */
+			if (r2_precise)
+				bt_clear_reg(bt, BPF_REG_2);
 			if (bt_reg_mask(bt) & BPF_REGMASK_ARGS) {
 				verifier_bug(env, "backtracking exit unexpected regs %x",
 					     bt_reg_mask(bt));
 				return -EFAULT;
 			}
 
-			/* BPF_EXIT in subprog or callback always returns
-			 * right after the call instruction, so by checking
-			 * whether the instruction at subseq_idx-1 is subprog
-			 * call or not we can distinguish actual exit from
-			 * *subprog* from exit from *callback*. In the former
-			 * case, we need to propagate r0 precision, if
-			 * necessary. In the former we never do that.
-			 */
-			r0_precise = subseq_idx - 1 >= 0 &&
-				     bpf_pseudo_call(&env->prog->insnsi[subseq_idx - 1]) &&
-				     bt_is_reg_set(bt, BPF_REG_0);
-
 			bt_clear_reg(bt, BPF_REG_0);
 			if (bt_subprog_enter(bt))
 				return -EFAULT;
 
 			if (r0_precise)
 				bt_set_reg(bt, BPF_REG_0);
+			if (r2_precise)
+				bt_set_reg(bt, BPF_REG_2);
 			/* r6-r9 and stack slots will stay set in caller frame
 			 * bitmasks until we return back from callee(s)
 			 */
-- 
2.53.0-Meta