[bpf] bpf: relax zero fixed offset constraint on KF_TRUSTED_ARGS/KF_RCU

Commit Message

Matt Bobrowski July 9, 2024, 9:09 p.m. UTC

Currently, BPF kfuncs which accept trusted pointer arguments
i.e. those flagged as KF_TRUSTED_ARGS, KF_RCU, or KF_RELEASE, all
require an original/unmodified trusted pointer argument to be supplied
to them. By original/unmodified, it means that the backing register
holding the trusted pointer argument that is to be supplied to the BPF
kfunc must have its fixed offset set to zero, or else the BPF verifier
will outright reject the BPF program load. However, this zero fixed
offset constraint that is currently enforced by the BPF verifier onto
BPF kfuncs specifically flagged to accept KF_TRUSTED_ARGS or KF_RCU
trusted pointer arguments is rather unnecessary, and can limit their
usability in practice. Specifically, it completely eliminates the
possibility of constructing a derived trusted pointer from an original
trusted pointer. To put it simply, a derived pointer is a pointer
which points to one of the nested member fields of the object being
pointed to by the original trusted pointer.

This patch relaxes the zero fixed offset constraint that is enforced
upon BPF kfuncs which specifically accept KF_TRUSTED_ARGS, or KF_RCU
arguments. Although, the zero fixed offset constraint technically also
applies to BPF kfuncs accepting KF_RELEASE arguments, relaxing this
constraint for such BPF kfuncs has subtle and unwanted
side-effects. This was discovered by experimenting a little further
with an initial version of this patch series [0]. The primary issue
with relaxing the zero fixed offset constraint on BPF kfuncs accepting
KF_RELEASE arguments is that it'd would open up the opportunity for
BPF programs to supply both trusted pointers and derived trusted
pointers to them. For KF_RELEASE BPF kfuncs specifically, this could
be problematic as resources associated with the backing pointer could
be released by the backing BPF kfunc and cause instabilities for the
rest of the kernel.

With this new fixed offset semantic in-place for BPF kfuncs accepting
KF_TRUSTED_ARGS and KF_RCU arguments, we now have more flexibility
when it comes to the BPF kfuncs that we're able to introduce moving
forward.

Early discussions covering the possibility of relaxing the zero fixed
offset constraint can be found using the link below. This will provide
more context on where all this has stemmed from [1].

Notably, pre-existing tests have been updated such that they provide
coverage for the updated zero fixed offset
functionality. Specifically, the nested offset test was converted from
a negative to positive test as it was already designed to assert zero
fixed offset semantics of a KF_TRUSTED_ARGS BPF kfunc.

[0] https://lore.kernel.org/bpf/[e-mail]/
[1] https://lore.kernel.org/bpf/[e-mail]/

Signed-off-by: Matt Bobrowski <[e-mail]>
---
 kernel/bpf/verifier.c                                    | 9 +++------
 tools/testing/selftests/bpf/progs/nested_trust_failure.c | 8 --------
 tools/testing/selftests/bpf/progs/nested_trust_success.c | 8 ++++++++
 tools/testing/selftests/bpf/verifier/calls.c             | 2 +-
 4 files changed, 12 insertions(+), 15 deletions(-)

Comments

Kumar Kartikeya Dwivedi July 9, 2024, 9:23 p.m. UTC | #1

On Tue, 9 Jul 2024 at 23:09, Matt Bobrowski <[e-mail]> wrote:
>
> Currently, BPF kfuncs which accept trusted pointer arguments
> i.e. those flagged as KF_TRUSTED_ARGS, KF_RCU, or KF_RELEASE, all
> require an original/unmodified trusted pointer argument to be supplied
> to them. By original/unmodified, it means that the backing register
> holding the trusted pointer argument that is to be supplied to the BPF
> kfunc must have its fixed offset set to zero, or else the BPF verifier
> will outright reject the BPF program load. However, this zero fixed
> offset constraint that is currently enforced by the BPF verifier onto
> BPF kfuncs specifically flagged to accept KF_TRUSTED_ARGS or KF_RCU
> trusted pointer arguments is rather unnecessary, and can limit their
> usability in practice. Specifically, it completely eliminates the
> possibility of constructing a derived trusted pointer from an original
> trusted pointer. To put it simply, a derived pointer is a pointer
> which points to one of the nested member fields of the object being
> pointed to by the original trusted pointer.
>
> This patch relaxes the zero fixed offset constraint that is enforced
> upon BPF kfuncs which specifically accept KF_TRUSTED_ARGS, or KF_RCU
> arguments. Although, the zero fixed offset constraint technically also
> applies to BPF kfuncs accepting KF_RELEASE arguments, relaxing this
> constraint for such BPF kfuncs has subtle and unwanted
> side-effects. This was discovered by experimenting a little further
> with an initial version of this patch series [0]. The primary issue
> with relaxing the zero fixed offset constraint on BPF kfuncs accepting
> KF_RELEASE arguments is that it'd would open up the opportunity for
> BPF programs to supply both trusted pointers and derived trusted
> pointers to them. For KF_RELEASE BPF kfuncs specifically, this could
> be problematic as resources associated with the backing pointer could
> be released by the backing BPF kfunc and cause instabilities for the
> rest of the kernel.
>
> With this new fixed offset semantic in-place for BPF kfuncs accepting
> KF_TRUSTED_ARGS and KF_RCU arguments, we now have more flexibility
> when it comes to the BPF kfuncs that we're able to introduce moving
> forward.
>
> Early discussions covering the possibility of relaxing the zero fixed
> offset constraint can be found using the link below. This will provide
> more context on where all this has stemmed from [1].
>
> Notably, pre-existing tests have been updated such that they provide
> coverage for the updated zero fixed offset
> functionality. Specifically, the nested offset test was converted from
> a negative to positive test as it was already designed to assert zero
> fixed offset semantics of a KF_TRUSTED_ARGS BPF kfunc.
>
> [0] https://lore.kernel.org/bpf/[e-mail]/
> [1] https://lore.kernel.org/bpf/[e-mail]/
>
> Signed-off-by: Matt Bobrowski <[e-mail]>
> ---

Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>

Though I'm not sure this is bpf material since it isn't a fix, it
might be better to base it against bpf-next.

[e-mail] July 10, 2024, 2:20 a.m. UTC | #2

Hello:

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

On Tue,  9 Jul 2024 21:09:39 +0000 you wrote:
> Currently, BPF kfuncs which accept trusted pointer arguments
> i.e. those flagged as KF_TRUSTED_ARGS, KF_RCU, or KF_RELEASE, all
> require an original/unmodified trusted pointer argument to be supplied
> to them. By original/unmodified, it means that the backing register
> holding the trusted pointer argument that is to be supplied to the BPF
> kfunc must have its fixed offset set to zero, or else the BPF verifier
> will outright reject the BPF program load. However, this zero fixed
> offset constraint that is currently enforced by the BPF verifier onto
> BPF kfuncs specifically flagged to accept KF_TRUSTED_ARGS or KF_RCU
> trusted pointer arguments is rather unnecessary, and can limit their
> usability in practice. Specifically, it completely eliminates the
> possibility of constructing a derived trusted pointer from an original
> trusted pointer. To put it simply, a derived pointer is a pointer
> which points to one of the nested member fields of the object being
> pointed to by the original trusted pointer.
> 
> [...]

Here is the summary with links:
  - [bpf] bpf: relax zero fixed offset constraint on KF_TRUSTED_ARGS/KF_RCU
    https://git.kernel.org/bpf/bpf-next/c/605c96997d89

You are awesome, thank you!

Matt Bobrowski July 10, 2024, 7:06 a.m. UTC | #3

On Tue, Jul 09, 2024 at 11:23:34PM +0200, Kumar Kartikeya Dwivedi wrote:
> On Tue, 9 Jul 2024 at 23:09, Matt Bobrowski <[e-mail]> wrote:
> >
> > Currently, BPF kfuncs which accept trusted pointer arguments
> > i.e. those flagged as KF_TRUSTED_ARGS, KF_RCU, or KF_RELEASE, all
> > require an original/unmodified trusted pointer argument to be supplied
> > to them. By original/unmodified, it means that the backing register
> > holding the trusted pointer argument that is to be supplied to the BPF
> > kfunc must have its fixed offset set to zero, or else the BPF verifier
> > will outright reject the BPF program load. However, this zero fixed
> > offset constraint that is currently enforced by the BPF verifier onto
> > BPF kfuncs specifically flagged to accept KF_TRUSTED_ARGS or KF_RCU
> > trusted pointer arguments is rather unnecessary, and can limit their
> > usability in practice. Specifically, it completely eliminates the
> > possibility of constructing a derived trusted pointer from an original
> > trusted pointer. To put it simply, a derived pointer is a pointer
> > which points to one of the nested member fields of the object being
> > pointed to by the original trusted pointer.
> >
> > This patch relaxes the zero fixed offset constraint that is enforced
> > upon BPF kfuncs which specifically accept KF_TRUSTED_ARGS, or KF_RCU
> > arguments. Although, the zero fixed offset constraint technically also
> > applies to BPF kfuncs accepting KF_RELEASE arguments, relaxing this
> > constraint for such BPF kfuncs has subtle and unwanted
> > side-effects. This was discovered by experimenting a little further
> > with an initial version of this patch series [0]. The primary issue
> > with relaxing the zero fixed offset constraint on BPF kfuncs accepting
> > KF_RELEASE arguments is that it'd would open up the opportunity for
> > BPF programs to supply both trusted pointers and derived trusted
> > pointers to them. For KF_RELEASE BPF kfuncs specifically, this could
> > be problematic as resources associated with the backing pointer could
> > be released by the backing BPF kfunc and cause instabilities for the
> > rest of the kernel.
> >
> > With this new fixed offset semantic in-place for BPF kfuncs accepting
> > KF_TRUSTED_ARGS and KF_RCU arguments, we now have more flexibility
> > when it comes to the BPF kfuncs that we're able to introduce moving
> > forward.
> >
> > Early discussions covering the possibility of relaxing the zero fixed
> > offset constraint can be found using the link below. This will provide
> > more context on where all this has stemmed from [1].
> >
> > Notably, pre-existing tests have been updated such that they provide
> > coverage for the updated zero fixed offset
> > functionality. Specifically, the nested offset test was converted from
> > a negative to positive test as it was already designed to assert zero
> > fixed offset semantics of a KF_TRUSTED_ARGS BPF kfunc.
> >
> > [0] https://lore.kernel.org/bpf/[e-mail]/
> > [1] https://lore.kernel.org/bpf/[e-mail]/
> >
> > Signed-off-by: Matt Bobrowski <[e-mail]>
> > ---
> 
> Acked-by: Kumar Kartikeya Dwivedi <[e-mail]>
> 
> Though I'm not sure this is bpf material since it isn't a fix, it
> might be better to base it against bpf-next.

Yes, sorry, this was based off bpf-next. I just happened to screw up
the subject prefix.

Thanks for the review! 

/M

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 3d6306c363b7..c0263fb5ca4b 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -11335,7 +11335,9 @@  static int process_kf_arg_ptr_to_btf_id(struct bpf_verifier_env *env,
 	    btf_type_ids_nocast_alias(&env->log, reg_btf, reg_ref_id, meta->btf, ref_id))
 		strict_type_match = true;
 
-	WARN_ON_ONCE(is_kfunc_trusted_args(meta) && reg->off);
+	WARN_ON_ONCE(is_kfunc_release(meta) &&
+		     (reg->off || !tnum_is_const(reg->var_off) ||
+		      reg->var_off.value));
 
 	reg_ref_t = btf_type_skip_modifiers(reg_btf, reg_ref_id, &reg_ref_id);
 	reg_ref_tname = btf_name_by_offset(reg_btf, reg_ref_t->name_off);
@@ -11917,12 +11919,8 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 					return -EINVAL;
 				}
 			}
-
 			fallthrough;
 		case KF_ARG_PTR_TO_CTX:
-			/* Trusted arguments have the same offset checks as release arguments */
-			arg_type |= OBJ_RELEASE;
-			break;
 		case KF_ARG_PTR_TO_DYNPTR:
 		case KF_ARG_PTR_TO_ITER:
 		case KF_ARG_PTR_TO_LIST_HEAD:
@@ -11935,7 +11933,6 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
 		case KF_ARG_PTR_TO_CONST_STR:
 		case KF_ARG_PTR_TO_WORKQUEUE:
-			/* Trusted by default */
 			break;
 		default:
 			WARN_ON_ONCE(1);
diff --git a/tools/testing/selftests/bpf/progs/nested_trust_failure.c b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
index ea39497f11ed..3568ec450100 100644
--- a/tools/testing/selftests/bpf/progs/nested_trust_failure.c
+++ b/tools/testing/selftests/bpf/progs/nested_trust_failure.c
@@ -31,14 +31,6 @@  int BPF_PROG(test_invalid_nested_user_cpus, struct task_struct *task, u64 clone_
 	return 0;
 }
 
-SEC("tp_btf/task_newtask")
-__failure __msg("R1 must have zero offset when passed to release func or trusted arg to kfunc")
-int BPF_PROG(test_invalid_nested_offset, struct task_struct *task, u64 clone_flags)
-{
-	bpf_cpumask_first_zero(&task->cpus_mask);
-	return 0;
-}
-
 /* Although R2 is of type sk_buff but sock_common is expected, we will hit untrusted ptr first. */
 SEC("tp_btf/tcp_probe")
 __failure __msg("R2 type=untrusted_ptr_ expected=ptr_, trusted_ptr_, rcu_ptr_")
diff --git a/tools/testing/selftests/bpf/progs/nested_trust_success.c b/tools/testing/selftests/bpf/progs/nested_trust_success.c
index 833840bffd3b..2b66953ca82e 100644
--- a/tools/testing/selftests/bpf/progs/nested_trust_success.c
+++ b/tools/testing/selftests/bpf/progs/nested_trust_success.c
@@ -32,3 +32,11 @@  int BPF_PROG(test_skb_field, struct sock *sk, struct sk_buff *skb)
 	bpf_sk_storage_get(&sk_storage_map, skb->sk, 0, 0);
 	return 0;
 }
+
+SEC("tp_btf/task_newtask")
+__success
+int BPF_PROG(test_nested_offset, struct task_struct *task, u64 clone_flags)
+{
+	bpf_cpumask_first_zero(&task->cpus_mask);
+	return 0;
+}
diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c
index ab25a81fd3a1..d76ef2018859 100644
--- a/tools/testing/selftests/bpf/verifier/calls.c
+++ b/tools/testing/selftests/bpf/verifier/calls.c
@@ -76,7 +76,7 @@ 
 	},
 	.prog_type = BPF_PROG_TYPE_SCHED_CLS,
 	.result = REJECT,
-	.errstr = "R1 must have zero offset when passed to release func or trusted arg to kfunc",
+	.errstr = "arg#0 expected pointer to ctx, but got PTR",
 	.fixup_kfunc_btf_id = {
 		{ "bpf_kfunc_call_test_pass_ctx", 2 },
 	},