[PATCH bpf-next 1/4] bpf: Keep fault protection when merging pointer types

[Daniel Borkmann <daniel@iogearbox.net>]

When the same BPF_LDX instruction is reached through paths that yield
different pointer types, save_aux_ptr_type() merges them into a single
type which is later used by bpf_convert_ctx_accesses() to decide whether
the load has to be rewritten into a BPF_PROBE_MEM one.

Before commit f2362a57aeff ("bpf: allow void* cast using bpf_rdonly_cast()")
the merge only accepted two PTR_TO_BTF_ID pointers and unconditionally
fell back to PTR_TO_BTF_ID | PTR_UNTRUSTED, so the merged type was always
one that gets the BPF_PROBE_MEM rewrite. However, the mentioned commit
widened the merge to also cover a PTR_TO_MEM base and replaced the
fallback by a union of the PTR_UNTRUSTED and MEM_RDONLY flags.

The union can produce types which bpf_convert_ctx_accesses() does not
rewrite, and the load then stays a plain one without an exception table
entry, e.g.:

  - PTR_TO_MEM merged with PTR_TO_BTF_ID | PTR_UNTRUSTED
   => PTR_TO_MEM | PTR_UNTRUSTED but only the MEM_RDONLY variant is valid
  - PTR_TO_MEM merged with a plain PTR_TO_BTF_ID
   => PTR_TO_MEM dropping the rewrite the latter type would have gotten
  - PTR_TO_MEM | MEM_RDONLY merged with a plain PTR_TO_BTF_ID
   => PTR_TO_MEM | MEM_RDONLY which is not rewritten either since only
    its PTR_UNTRUSTED variant is

In all three cases a program can take the unsafe path at runtime with a
NULL or otherwise bad pointer and panic the kernel on the faulting load.
Fix it by normalizing the merged type: if either side needs the rewrite,
pick the one rewritten form the merged base type has.

Reuse the may_fault_on_deref() helper in is_load_acq_unsafe() as well to
avoid open coding, and trim the overly verbose comment which is more of
implementation detail of bpf_convert_ctx_accesses() anyway.

Fixes: f2362a57aeff ("bpf: allow void* cast using bpf_rdonly_cast()")
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
---
 kernel/bpf/verifier.c | 61 +++++++++++++++++++++++++------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 61ef43325c6f..0d3b76d7820e 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4819,6 +4819,18 @@ static bool is_arena_reg(struct bpf_verifier_env *env, int regno)
 	return reg->type == PTR_TO_ARENA;
 }
 
+static bool may_fault_on_deref(enum bpf_reg_type type)
+{
+	/*
+	 * The pointer types which must not be dereferenced without fault
+	 * protection, that is, the ones bpf_convert_ctx_accesses() has to
+	 * turn a BPF_LDX into a BPF_PROBE_MEM one for. Slightly wider than
+	 * the list matched there, which relies on an untrusted PTR_TO_MEM
+	 * always carrying MEM_RDONLY as well.
+	 */
+	return type == PTR_TO_BTF_ID || (type_flag(type) & PTR_UNTRUSTED);
+}
+
 static bool is_load_acq_unsafe(struct bpf_verifier_env *env, int regno,
 			       struct bpf_insn *insn)
 {
@@ -4828,19 +4840,11 @@ static bool is_load_acq_unsafe(struct bpf_verifier_env *env, int regno,
 	 * A BPF_LOAD_ACQ is not rewritten to a BPF_PROBE_MEM load by the
 	 * verifier, unlike a regular BPF_LDX. The JIT would emit a plain load
 	 * with no exception table entry, so a fault (e.g. NULL deref) crashes
-	 * the kernel instead of being handled.
-	 *
-	 * Reject the source pointer types that a BPF_LDX would have had that
-	 * fault protection applied to, i.e. the ones bpf_convert_ctx_accesses()
-	 * turns into BPF_PROBE_MEM: a bare PTR_TO_BTF_ID and any PTR_UNTRUSTED
-	 * pointer (untrusted btf ids, untrusted MEM_ALLOC, rdonly untrusted
-	 * memory). A PTR_TRUSTED pointer is not among them, is not converted,
-	 * and stays allowed. Same for the other flagged PTR_TO_BTF_ID variants
-	 * (MEM_ALLOC, MEM_RCU, ...), hence the exact match on the base type.
+	 * the kernel instead of being handled. Reject the source pointer types
+	 * that would have needed that protection, the remaining ones stay
+	 * allowed.
 	 */
-	return insn->imm == BPF_LOAD_ACQ &&
-	       (reg->type == PTR_TO_BTF_ID ||
-		(type_flag(reg->type) & PTR_UNTRUSTED));
+	return insn->imm == BPF_LOAD_ACQ && may_fault_on_deref(reg->type);
 }
 
 /* Return false if @regno contains a pointer whose type isn't supported for
@@ -17021,11 +17025,24 @@ static bool is_ptr_to_mem(enum bpf_reg_type type)
 	return base_type(type) == PTR_TO_MEM;
 }
 
+static enum bpf_reg_type merge_ptr_types(enum bpf_reg_type type_a,
+					 enum bpf_reg_type type_b)
+{
+	bool to_mem = is_ptr_to_mem(type_a) || is_ptr_to_mem(type_b);
+	enum bpf_reg_type type_merged = to_mem ? PTR_TO_MEM : PTR_TO_BTF_ID;
+
+	if (may_fault_on_deref(type_a) || may_fault_on_deref(type_b))
+		type_merged |= to_mem ? MEM_RDONLY | PTR_UNTRUSTED :
+					PTR_UNTRUSTED;
+	else
+		type_merged |= ((type_a | type_b) & MEM_RDONLY);
+	return type_merged;
+}
+
 static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type type,
 			     bool allow_trust_mismatch)
 {
 	enum bpf_reg_type *prev_type = &env->insn_aux_data[env->insn_idx].ptr_type;
-	enum bpf_reg_type merged_type;
 
 	if (*prev_type == NOT_INIT) {
 		/* Saw a valid insn
@@ -17046,20 +17063,12 @@ static int save_aux_ptr_type(struct bpf_verifier_env *env, enum bpf_reg_type typ
 		    is_ptr_to_mem_or_btf_id(*prev_type)) {
 			/*
 			 * Have to support a use case when one path through
-			 * the program yields TRUSTED pointer while another
-			 * is UNTRUSTED. Fallback to UNTRUSTED to generate
-			 * BPF_PROBE_MEM/BPF_PROBE_MEMSX.
-			 * Same behavior of MEM_RDONLY flag.
+			 * the program yields a TRUSTED pointer while another
+			 * is UNTRUSTED. Merge them into a type which keeps
+			 * the BPF_PROBE_MEM/BPF_PROBE_MEMSX rewrite when
+			 * either side needs it.
 			 */
-			if (is_ptr_to_mem(type) || is_ptr_to_mem(*prev_type))
-				merged_type = PTR_TO_MEM;
-			else
-				merged_type = PTR_TO_BTF_ID;
-			if ((type & PTR_UNTRUSTED) || (*prev_type & PTR_UNTRUSTED))
-				merged_type |= PTR_UNTRUSTED;
-			if ((type & MEM_RDONLY) || (*prev_type & MEM_RDONLY))
-				merged_type |= MEM_RDONLY;
-			*prev_type = merged_type;
+			*prev_type = merge_ptr_types(type, *prev_type);
 		} else {
 			verbose(env, "same insn cannot be used with different pointers\n");
 			return -EINVAL;
-- 
2.43.0