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 looks at sanitize_check_bounds().
However, this is misleading as the function 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, 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
  Spectre v4) that goes into the alu op. The ptr reg can not be subject
  to v4-based value confusion due to the nospec added. Thus, in this
  case it would have been fine to also skip sanitize_check_bounds().

* We are on a speculative path (`vstate->speculative`). In the second
  "commit" phase, sanitize_ptr_alu() always just returns 0. This makes
  sense because there are no alu sanitization limits to be learned from
  speculative paths. Furthermore, because the sanitization will ensure
  that ptr arith stays in (architectural) bounds, the
  sanitize_check_bounds() on the spec. path could also be skipped.

TODO, through case two, is it fine that we allow alu with other ptr type
(non-stack/ap) on spec path

retrieve_ptr_limit() only allows the ALU op if the involved ptr reg (can
be either src or dst 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() also
runs for all relevant cases and prevents unsafe ops.

We therefore allow unsanitized ptr arith with 64-bit 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 between sanitize_check_bounds() and
retrieve_ptr_limit(), add a verifier_bug_if() to make sure they stay in
sync.

Further background info:

* There are two calls to sanitize_ptr_alu(), on the first call we spawn
  the path to simulate truncation and

TODO, why spawn the path before simulating the alu op
TODO, why two calls to sanitize_ptr_alu

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,21 @@ 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 (verifier_bug_if(!sanitize_needed(opcode), env, "x55")) {
+		return -EFAULT;
+	}
 	if (sanitize_needed(opcode)) {
 		ret = sanitize_ptr_alu(env, insn, dst_reg, off_reg, dst_reg,
 				       &info, true);
+		if (verifier_bug_if(!can_skip_alu_sanitation(env, insn)
+				    && !env->cur_state->speculative /* TODO: ok? */
+				    && 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);
 	}