bpf: Clarify sanitize_check_bounds()

As is, it appears as if ptr alu is allowed for everything except
PTR_TO_{STACK,MAP_VALUE} if one only reads sanitize_check_bounds().
However, this is misleading as it only works together with
retrieve_ptr_limit() and the two must be kept in sync. This patch
documents the interdependency and adds a check to ensure they stay in
sync.

Because the preceeding switch returns -EACCES for every opcode except
for ADD/SUB, the sanitize_needed() following the sanitize_check_bounds()
call is always true if reached. This means, unless
sanitize_check_bounds() detected that the pointer goes OOB because of
the ADD/SUB (and returns -EACCES), sanitize_ptr_alu() always executes
after sanitize_check_bounds().

In sanitize_ptr_alu(commit_window = true), we always run
retrieve_ptr_limit() unless:
* can_skip_alu_sanititation() is true, i.e., noteably
  `BPF_SRC(insn->code) == BPF_K`. BPF_K is fine because it means that
  there is no scalar register (which could be subject to spec. scalar
  confusion due to v4) that goes into the alu op. The ptr reg can not be
  subject to v4-based value confusion due to the nospec added.
* We are on a speculative path (`vstate->speculative`). This is because
  there are no alu sanitization limits to be learned from speculative
  paths.

retrieve_ptr_limit() only allows the ALU op if the involved ptr reg (can
be either rhs or lhs for ADD) is PTR_TO_STACK or PTR_TO_MAP_VALUE.
Otherwise it returns -EOPNOTSUPP.

In summary, sanitize_check_bounds() returning 0 for
non-PTR_TO_{STACK,MAP_VALUE} is fine because retrieve_ptr_limit() still
prevents the unsafe ops.

We allow unsanitized ptr arith with ADD/SUB for
* ptr -=/+= imm32, i.e. `BPF_SRC(insn->code) == BPF_K`
* PTR_TO_{STACK,MAP_VALUE} -= scalar
* PTR_TO_{STACK,MAP_VALUE} += scalar
* scalar += PTR_TO_{STACK,MAP_VALUE}
if the requirements from retrieve_ptr_limit() AND
sanitize_check_bounds() hold.

To document this interdependency and ensure they stay in sync, add a
verifier_bug_if().

Signed-off-by: Luis Gerhorst <[email protected]>

Author: luisgerhorst

kernel/bpf/verifier.c

@@ -14297,7 +14297,7 @@ static int sanitize_check_bounds(struct bpf_verifier_env *env,
 		}
 		break;
 	default:
-		break;
+		return -EOPNOTSUPP;
 	}
 
 	return 0;
@@ -14324,7 +14324,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	struct bpf_sanitize_info info = {};
 	u8 opcode = BPF_OP(insn->code);
 	u32 dst = insn->dst_reg;
-	int ret;
+	int ret, bounds_ret;
 
 	dst_reg = &regs[dst];
 
@@ -14524,11 +14524,17 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	if (!check_reg_sane_offset(env, dst_reg, ptr_reg->type))
 		return -EINVAL;
 	reg_bounds_sync(dst_reg);
-	if (sanitize_check_bounds(env, insn, dst_reg) < 0)
-		return -EACCES;
+	bounds_ret = sanitize_check_bounds(env, insn, dst_reg);
+	if (bounds_ret == -EACCES)
+		return bounds_ret;
 	if (sanitize_needed(opcode)) {
 		ret = sanitize_ptr_alu(env, insn, dst_reg, off_reg, dst_reg,
 				       &info, true);
+		if (!can_skip_alu_sanitation(env, insn) &&
+		    verifier_bug_if(bounds_ret == -EOPNOTSUPP && !ret,
+				    env, "Pointer type unsupported by sanitize_check_bounds() not rejected by retrieve_ptr_limit() as required")) {
+			return -EFAULT;
+		}
 		if (ret < 0)
 			return sanitize_err(env, insn, ret, off_reg, dst_reg);
 	}