[bpf-next] bpf: Don't mark arguments to fentry/fexit programs as trusted.

Commit Message

Alexei Starovoitov Nov. 24, 2022, 9:53 p.m. UTC

From: Alexei Starovoitov <[e-mail]>

The PTR_TRUSTED flag should only be applied to pointers where the verifier can
guarantee that such pointers are valid.
The fentry/fexit/fmod_ret programs are not in this category.
Only arguments of SEC("tp_btf") and SEC("iter") programs are trusted
(which have BPF_TRACE_RAW_TP and BPF_TRACE_ITER attach_type correspondingly)

This bug was masked because convert_ctx_accesses() was converting trusted
loads into BPF_PROBE_MEM loads. Fix it as well.
The loads from trusted pointers don't need exception handling.

Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
Signed-off-by: Alexei Starovoitov <[e-mail]>
---
 kernel/bpf/btf.c      | 16 +++++++++++++---
 kernel/bpf/verifier.c |  3 ---
 2 files changed, 13 insertions(+), 6 deletions(-)

Comments

[e-mail] Nov. 24, 2022, 10:50 p.m. UTC | #1

Hello:

This patch was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <[e-mail]>:

On Thu, 24 Nov 2022 13:53:14 -0800 you wrote:
> From: Alexei Starovoitov <[e-mail]>
> 
> The PTR_TRUSTED flag should only be applied to pointers where the verifier can
> guarantee that such pointers are valid.
> The fentry/fexit/fmod_ret programs are not in this category.
> Only arguments of SEC("tp_btf") and SEC("iter") programs are trusted
> (which have BPF_TRACE_RAW_TP and BPF_TRACE_ITER attach_type correspondingly)
> 
> [...]

Here is the summary with links:
  - [bpf-next] bpf: Don't mark arguments to fentry/fexit programs as trusted.
    https://git.kernel.org/bpf/bpf-next/c/c6b0337f0120

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index bd3369100239..d11cbf8cece7 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -5821,9 +5821,19 @@  static u32 get_ctx_arg_idx(struct btf *btf, const struct btf_type *func_proto,
 	return nr_args + 1;
 }
 
-static bool prog_type_args_trusted(enum bpf_prog_type prog_type)
+static bool prog_args_trusted(const struct bpf_prog *prog)
 {
-	return prog_type == BPF_PROG_TYPE_TRACING || prog_type == BPF_PROG_TYPE_STRUCT_OPS;
+	enum bpf_attach_type atype = prog->expected_attach_type;
+
+	switch (prog->type) {
+	case BPF_PROG_TYPE_TRACING:
+		return atype == BPF_TRACE_RAW_TP || atype == BPF_TRACE_ITER;
+	case BPF_PROG_TYPE_LSM:
+	case BPF_PROG_TYPE_STRUCT_OPS:
+		return true;
+	default:
+		return false;
+	}
 }
 
 bool btf_ctx_access(int off, int size, enum bpf_access_type type,
@@ -5969,7 +5979,7 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	}
 
 	info->reg_type = PTR_TO_BTF_ID;
-	if (prog_type_args_trusted(prog->type))
+	if (prog_args_trusted(prog))
 		info->reg_type |= PTR_TRUSTED;
 
 	if (tgt_prog) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index f4500479f1c2..6599d25dae38 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -14905,7 +14905,6 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 			break;
 		case PTR_TO_BTF_ID:
 		case PTR_TO_BTF_ID | PTR_UNTRUSTED:
-		case PTR_TO_BTF_ID | PTR_TRUSTED:
 		/* PTR_TO_BTF_ID | MEM_ALLOC always has a valid lifetime, unlike
 		 * PTR_TO_BTF_ID, and an active ref_obj_id, but the same cannot
 		 * be said once it is marked PTR_UNTRUSTED, hence we must handle
@@ -14913,8 +14912,6 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		 * for this case.
 		 */
 		case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
-		case PTR_TO_BTF_ID | PTR_UNTRUSTED | PTR_TRUSTED:
-		case PTR_TO_BTF_ID | PTR_UNTRUSTED | MEM_ALLOC | PTR_TRUSTED:
 			if (type == BPF_READ) {
 				insn->code = BPF_LDX | BPF_PROBE_MEM |
 					BPF_SIZE((insn)->code);