From: Luis Gerhorst Date: Tue, 3 Jun 2025 20:45:57 +0000 (+0200) Subject: bpf: Clarify sanitize_check_bounds() X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=97744b4971d81bf7336dfd782a08188c9e09f4ee;p=thirdparty%2Fkernel%2Flinux.git bpf: Clarify sanitize_check_bounds() As is, it appears as if pointer arithmetic 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. adjust_ptr_min_max_vals(): Because the preceding 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(). The following shows that this also implies that retrieve_ptr_limit() runs in all relevant cases. Note that there are two calls to sanitize_ptr_alu(), these are simply needed to easily calculate the correct alu_limit as explained in commit 7fedb63a8307 ("bpf: Tighten speculative pointer arithmetic mask"). The truncation-simulation is already performed on the first call. In the second sanitize_ptr_alu(commit_window = true), we always run retrieve_ptr_limit(), unless: * can_skip_alu_sanititation() is true, notably `BPF_SRC(insn->code) == BPF_K`. BPF_K is fine because it means that there is no scalar register (which could be subject to speculative scalar confusion due to Spectre v4) that goes into the ALU operation. The pointer register 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(). * If we are on a speculative path (`vstate->speculative`) and 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 pointer arithmetic stays in (architectural) bounds, the sanitize_check_bounds() on the speculative path could also be skipped. The second case needs more attention: Assume we have some ALU operation that is used with scalars architecturally, but with a non-PTR_TO_{STACK,MAP_VALUE} pointer (e.g., PTR_TO_PACKET) speculatively. It might appear as if this would allow an unsanitized pointer ALU operations, but this can not happen because one of the following two always holds: * The type mismatch stems from Spectre v4, then it is prevented by a nospec after the possibly-bypassed store involving the pointer. There is no speculative path simulated for this case thus it never happens. * The type mismatch stems from a Spectre v1 gadget like the following: r1 = slow(0) r4 = fast(0) r3 = SCALAR // Spectre v4 scalar confusion if (r1) { r2 = PTR_TO_PACKET } else { r2 = 42 } if (r4) { r2 += r3 *r2 } If `r2 = PTR_TO_PACKET` is indeed dead code, it will be sanitized to `goto -1` (as is the case for the r4-if block). If it is not (e.g., if `r1 = r4 = 1` is possible), it will also be explored on an architectural path and retrieve_ptr_limit() will reject it. To summarize, the exception for `vstate->speculative` is safe. Back to retrieve_ptr_limit(): It only allows the ALU operation if the involved pointer register (can be either source or destination for ADD) is PTR_TO_STACK or PTR_TO_MAP_VALUE. Otherwise, it returns -EOPNOTSUPP. Therefore, 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 operations. To summarize, we allow unsanitized pointer arithmetic with 64-bit ADD/SUB for the following instructions if the requirements from retrieve_ptr_limit() AND sanitize_check_bounds() hold: * 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} To document the interdependency between sanitize_check_bounds() and retrieve_ptr_limit(), add a verifier_bug_if() to make sure they stay in sync. Signed-off-by: Luis Gerhorst Reported-by: Kumar Kartikeya Dwivedi Link: https://lore.kernel.org/bpf/CAP01T76HZ+s5h+_REqRFkRjjoKwnZZn9YswpSVinGicah1pGJw@mail.gmail.com/ Link: https://lore.kernel.org/bpf/CAP01T75oU0zfZCiymEcH3r-GQ5A6GOc6GmYzJEnMa3=53XuUQQ@mail.gmail.com/ Link: https://lore.kernel.org/r/20250603204557.332447-1-luis.gerhorst@fau.de Signed-off-by: Alexei Starovoitov --- diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a7d6e0c5928bc..e31f6b0ccb30e 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -14284,7 +14284,7 @@ static int sanitize_check_bounds(struct bpf_verifier_env *env, } break; default: - break; + return -EOPNOTSUPP; } return 0; @@ -14311,7 +14311,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 = ®s[dst]; @@ -14511,11 +14511,19 @@ 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 (verifier_bug_if(!can_skip_alu_sanitation(env, insn) + && !env->cur_state->speculative + && bounds_ret + && !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); }