diff mbox series

[bpf-next,v7,04/13] bpf: Add support for forcing kfunc args to be trusted

Message ID 20220721134245.2450-5-memxor@gmail.com (mailing list archive)
State Accepted
Commit 56e948ffc098a780fefb6c1784a3a2c7b81100a1
Delegated to: BPF
Headers show
Series New nf_conntrack kfuncs for insertion, changing timeout, status | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
bpf/vmtest-bpf-next-VM_Test-1 success Logs for Kernel LATEST on ubuntu-latest with gcc
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Kernel LATEST on ubuntu-latest with llvm-15
bpf/vmtest-bpf-next-VM_Test-3 success Logs for Kernel LATEST on z15 with gcc
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit fail Errors and warnings before: 1424 this patch: 1389
netdev/cc_maintainers warning 12 maintainers not CCed: davem@davemloft.net song@kernel.org jolsa@kernel.org martin.lau@linux.dev john.fastabend@gmail.com yhs@fb.com sdf@google.com pabeni@redhat.com kpsingh@kernel.org haoluo@google.com edumazet@google.com kuba@kernel.org
netdev/build_clang fail Errors and warnings before: 216 this patch: 218
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn fail Errors and warnings before: 1433 this patch: 1436
netdev/checkpatch warning CHECK: Prefer using the BIT macro WARNING: line length of 81 exceeds 80 columns WARNING: line length of 83 exceeds 80 columns WARNING: line length of 93 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Kumar Kartikeya Dwivedi July 21, 2022, 1:42 p.m. UTC
Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
means each pointer argument must be trusted, which we define as a
pointer that is referenced (has non-zero ref_obj_id) and also needs to
have its offset unchanged, similar to how release functions expect their
argument. This allows a kfunc to receive pointer arguments unchanged
from the result of the acquire kfunc.

This is required to ensure that kfunc that operate on some object only
work on acquired pointers and not normal PTR_TO_BTF_ID with same type
which can be obtained by pointer walking. The restrictions applied to
release arguments also apply to trusted arguments. This implies that
strict type matching (not deducing type by recursively following members
at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
used for trusted pointer arguments.

Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
 kernel/bpf/btf.c    | 17 ++++++++++++++---
 net/bpf/test_run.c  |  5 +++++
 3 files changed, 51 insertions(+), 3 deletions(-)

Comments

Alexei Starovoitov July 22, 2022, 4:10 a.m. UTC | #1
On Thu, Jul 21, 2022 at 03:42:36PM +0200, Kumar Kartikeya Dwivedi wrote:
> +/* Trusted arguments are those which are meant to be referenced arguments with
> + * unchanged offset. It is used to enforce that pointers obtained from acquire
> + * kfuncs remain unmodified when being passed to helpers taking trusted args.
> + *
> + * Consider
> + *	struct foo {
> + *		int data;
> + *		struct foo *next;
> + *	};
> + *
> + *	struct bar {
> + *		int data;
> + *		struct foo f;
> + *	};
> + *
> + *	struct foo *f = alloc_foo(); // Acquire kfunc
> + *	struct bar *b = alloc_bar(); // Acquire kfunc
> + *
> + * If a kfunc set_foo_data() wants to operate only on the allocated object, it
> + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> + *
> + *	set_foo_data(f, 42);	   // Allowed
> + *	set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> + *	set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> + *	set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type

I think you meant to swap above two comments ?
That's what I did while applying.

Also fixed typo in Fixes tag in patch 13. It was missing a letter in sha.

Since there are 3 other pending patchsets in patchwork that add new kfuncs
this cleanup of kfunc registration couldn't have come at better time.

Thank you for doing this work.
Kumar Kartikeya Dwivedi July 22, 2022, 10:26 a.m. UTC | #2
On Fri, 22 Jul 2022 at 06:11, Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Thu, Jul 21, 2022 at 03:42:36PM +0200, Kumar Kartikeya Dwivedi wrote:
> > +/* Trusted arguments are those which are meant to be referenced arguments with
> > + * unchanged offset. It is used to enforce that pointers obtained from acquire
> > + * kfuncs remain unmodified when being passed to helpers taking trusted args.
> > + *
> > + * Consider
> > + *   struct foo {
> > + *           int data;
> > + *           struct foo *next;
> > + *   };
> > + *
> > + *   struct bar {
> > + *           int data;
> > + *           struct foo f;
> > + *   };
> > + *
> > + *   struct foo *f = alloc_foo(); // Acquire kfunc
> > + *   struct bar *b = alloc_bar(); // Acquire kfunc
> > + *
> > + * If a kfunc set_foo_data() wants to operate only on the allocated object, it
> > + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> > + *
> > + *   set_foo_data(f, 42);       // Allowed
> > + *   set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> > + *   set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> > + *   set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
>
> I think you meant to swap above two comments ?
> That's what I did while applying.
>
> Also fixed typo in Fixes tag in patch 13. It was missing a letter in sha.
>
> Since there are 3 other pending patchsets in patchwork that add new kfuncs
> this cleanup of kfunc registration couldn't have come at better time.
>
> Thank you for doing this work.

Thank you for doing the fixups!
Roberto Sassu July 25, 2022, 9:52 a.m. UTC | #3
> From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> Sent: Thursday, July 21, 2022 3:43 PM
> Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
> means each pointer argument must be trusted, which we define as a
> pointer that is referenced (has non-zero ref_obj_id) and also needs to
> have its offset unchanged, similar to how release functions expect their
> argument. This allows a kfunc to receive pointer arguments unchanged
> from the result of the acquire kfunc.
> 
> This is required to ensure that kfunc that operate on some object only
> work on acquired pointers and not normal PTR_TO_BTF_ID with same type
> which can be obtained by pointer walking. The restrictions applied to
> release arguments also apply to trusted arguments. This implies that
> strict type matching (not deducing type by recursively following members
> at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
> used for trusted pointer arguments.
> 
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
>  kernel/bpf/btf.c    | 17 ++++++++++++++---
>  net/bpf/test_run.c  |  5 +++++
>  3 files changed, 51 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/btf.h b/include/linux/btf.h
> index 6dfc6eaf7f8c..cb63aa71e82f 100644
> --- a/include/linux/btf.h
> +++ b/include/linux/btf.h
> @@ -17,6 +17,38 @@
>  #define KF_RELEASE	(1 << 1) /* kfunc is a release function */
>  #define KF_RET_NULL	(1 << 2) /* kfunc returns a pointer that may be NULL */
>  #define KF_KPTR_GET	(1 << 3) /* kfunc returns reference to a kptr */
> +/* Trusted arguments are those which are meant to be referenced arguments
> with
> + * unchanged offset. It is used to enforce that pointers obtained from acquire
> + * kfuncs remain unmodified when being passed to helpers taking trusted args.
> + *
> + * Consider
> + *	struct foo {
> + *		int data;
> + *		struct foo *next;
> + *	};
> + *
> + *	struct bar {
> + *		int data;
> + *		struct foo f;
> + *	};
> + *
> + *	struct foo *f = alloc_foo(); // Acquire kfunc
> + *	struct bar *b = alloc_bar(); // Acquire kfunc
> + *
> + * If a kfunc set_foo_data() wants to operate only on the allocated object, it
> + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> + *
> + *	set_foo_data(f, 42);	   // Allowed
> + *	set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> + *	set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> + *	set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
> + *
> + * In the final case, usually for the purposes of type matching, it is deduced
> + * by looking at the type of the member at the offset, but due to the
> + * requirement of trusted argument, this deduction will be strict and not done
> + * for this case.
> + */
> +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> arguments */

Hi Kumar

would it make sense to introduce per-parameter flags? I have a function
that has several parameters, but only one is referenced.

Thanks

Roberto
Kumar Kartikeya Dwivedi July 26, 2022, 9:30 a.m. UTC | #4
On Mon, 25 Jul 2022 at 11:52, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > Sent: Thursday, July 21, 2022 3:43 PM
> > Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
> > means each pointer argument must be trusted, which we define as a
> > pointer that is referenced (has non-zero ref_obj_id) and also needs to
> > have its offset unchanged, similar to how release functions expect their
> > argument. This allows a kfunc to receive pointer arguments unchanged
> > from the result of the acquire kfunc.
> >
> > This is required to ensure that kfunc that operate on some object only
> > work on acquired pointers and not normal PTR_TO_BTF_ID with same type
> > which can be obtained by pointer walking. The restrictions applied to
> > release arguments also apply to trusted arguments. This implies that
> > strict type matching (not deducing type by recursively following members
> > at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
> > used for trusted pointer arguments.
> >
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
> >  kernel/bpf/btf.c    | 17 ++++++++++++++---
> >  net/bpf/test_run.c  |  5 +++++
> >  3 files changed, 51 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 6dfc6eaf7f8c..cb63aa71e82f 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -17,6 +17,38 @@
> >  #define KF_RELEASE   (1 << 1) /* kfunc is a release function */
> >  #define KF_RET_NULL  (1 << 2) /* kfunc returns a pointer that may be NULL */
> >  #define KF_KPTR_GET  (1 << 3) /* kfunc returns reference to a kptr */
> > +/* Trusted arguments are those which are meant to be referenced arguments
> > with
> > + * unchanged offset. It is used to enforce that pointers obtained from acquire
> > + * kfuncs remain unmodified when being passed to helpers taking trusted args.
> > + *
> > + * Consider
> > + *   struct foo {
> > + *           int data;
> > + *           struct foo *next;
> > + *   };
> > + *
> > + *   struct bar {
> > + *           int data;
> > + *           struct foo f;
> > + *   };
> > + *
> > + *   struct foo *f = alloc_foo(); // Acquire kfunc
> > + *   struct bar *b = alloc_bar(); // Acquire kfunc
> > + *
> > + * If a kfunc set_foo_data() wants to operate only on the allocated object, it
> > + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> > + *
> > + *   set_foo_data(f, 42);       // Allowed
> > + *   set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> > + *   set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> > + *   set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
> > + *
> > + * In the final case, usually for the purposes of type matching, it is deduced
> > + * by looking at the type of the member at the offset, but due to the
> > + * requirement of trusted argument, this deduction will be strict and not done
> > + * for this case.
> > + */
> > +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > arguments */
>
> Hi Kumar
>
> would it make sense to introduce per-parameter flags? I have a function
> that has several parameters, but only one is referenced.
>

I have a patch for that in my local branch, I can fix it up and post
it. But first, can you give an example of where you think you need it?

> Thanks
>
> Roberto
Roberto Sassu July 26, 2022, 10:02 a.m. UTC | #5
> From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> Sent: Tuesday, July 26, 2022 11:30 AM
> On Mon, 25 Jul 2022 at 11:52, Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > Sent: Thursday, July 21, 2022 3:43 PM
> > > Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
> > > means each pointer argument must be trusted, which we define as a
> > > pointer that is referenced (has non-zero ref_obj_id) and also needs to
> > > have its offset unchanged, similar to how release functions expect their
> > > argument. This allows a kfunc to receive pointer arguments unchanged
> > > from the result of the acquire kfunc.
> > >
> > > This is required to ensure that kfunc that operate on some object only
> > > work on acquired pointers and not normal PTR_TO_BTF_ID with same type
> > > which can be obtained by pointer walking. The restrictions applied to
> > > release arguments also apply to trusted arguments. This implies that
> > > strict type matching (not deducing type by recursively following members
> > > at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
> > > used for trusted pointer arguments.
> > >
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
> > >  kernel/bpf/btf.c    | 17 ++++++++++++++---
> > >  net/bpf/test_run.c  |  5 +++++
> > >  3 files changed, 51 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > index 6dfc6eaf7f8c..cb63aa71e82f 100644
> > > --- a/include/linux/btf.h
> > > +++ b/include/linux/btf.h
> > > @@ -17,6 +17,38 @@
> > >  #define KF_RELEASE   (1 << 1) /* kfunc is a release function */
> > >  #define KF_RET_NULL  (1 << 2) /* kfunc returns a pointer that may be NULL
> */
> > >  #define KF_KPTR_GET  (1 << 3) /* kfunc returns reference to a kptr */
> > > +/* Trusted arguments are those which are meant to be referenced
> arguments
> > > with
> > > + * unchanged offset. It is used to enforce that pointers obtained from
> acquire
> > > + * kfuncs remain unmodified when being passed to helpers taking trusted
> args.
> > > + *
> > > + * Consider
> > > + *   struct foo {
> > > + *           int data;
> > > + *           struct foo *next;
> > > + *   };
> > > + *
> > > + *   struct bar {
> > > + *           int data;
> > > + *           struct foo f;
> > > + *   };
> > > + *
> > > + *   struct foo *f = alloc_foo(); // Acquire kfunc
> > > + *   struct bar *b = alloc_bar(); // Acquire kfunc
> > > + *
> > > + * If a kfunc set_foo_data() wants to operate only on the allocated object,
> it
> > > + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> > > + *
> > > + *   set_foo_data(f, 42);       // Allowed
> > > + *   set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> > > + *   set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> > > + *   set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
> > > + *
> > > + * In the final case, usually for the purposes of type matching, it is deduced
> > > + * by looking at the type of the member at the offset, but due to the
> > > + * requirement of trusted argument, this deduction will be strict and not
> done
> > > + * for this case.
> > > + */
> > > +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > > arguments */
> >
> > Hi Kumar
> >
> > would it make sense to introduce per-parameter flags? I have a function
> > that has several parameters, but only one is referenced.
> >
> 
> I have a patch for that in my local branch, I can fix it up and post
> it. But first, can you give an example of where you think you need it?

I have pushed the complete patch set here, for testing:

https://github.com/robertosassu/vmtest/tree/bpf-verify-sig-v9/travis-ci/diffs

I rebased to bpf-next/master, and introduced KF_SLEEPABLE (similar
functionality of " btf: Add a new kfunc set which allows to mark
a function to be sleepable" from Benjamin Tissoires).

The patch where I would use per-parameter KF_TRUSTED_ARGS is
number 8. I also used your new API in patch 7 and it works well.

I didn't repost, as I'm waiting for comments on v8.

Thanks

Roberto
Kumar Kartikeya Dwivedi July 26, 2022, 12:55 p.m. UTC | #6
On Tue, 26 Jul 2022 at 12:02, Roberto Sassu <roberto.sassu@huawei.com> wrote:
>
> > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > Sent: Tuesday, July 26, 2022 11:30 AM
> > On Mon, 25 Jul 2022 at 11:52, Roberto Sassu <roberto.sassu@huawei.com>
> > wrote:
> > >
> > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > Sent: Thursday, July 21, 2022 3:43 PM
> > > > Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
> > > > means each pointer argument must be trusted, which we define as a
> > > > pointer that is referenced (has non-zero ref_obj_id) and also needs to
> > > > have its offset unchanged, similar to how release functions expect their
> > > > argument. This allows a kfunc to receive pointer arguments unchanged
> > > > from the result of the acquire kfunc.
> > > >
> > > > This is required to ensure that kfunc that operate on some object only
> > > > work on acquired pointers and not normal PTR_TO_BTF_ID with same type
> > > > which can be obtained by pointer walking. The restrictions applied to
> > > > release arguments also apply to trusted arguments. This implies that
> > > > strict type matching (not deducing type by recursively following members
> > > > at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
> > > > used for trusted pointer arguments.
> > > >
> > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > ---
> > > >  include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
> > > >  kernel/bpf/btf.c    | 17 ++++++++++++++---
> > > >  net/bpf/test_run.c  |  5 +++++
> > > >  3 files changed, 51 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > > index 6dfc6eaf7f8c..cb63aa71e82f 100644
> > > > --- a/include/linux/btf.h
> > > > +++ b/include/linux/btf.h
> > > > @@ -17,6 +17,38 @@
> > > >  #define KF_RELEASE   (1 << 1) /* kfunc is a release function */
> > > >  #define KF_RET_NULL  (1 << 2) /* kfunc returns a pointer that may be NULL
> > */
> > > >  #define KF_KPTR_GET  (1 << 3) /* kfunc returns reference to a kptr */
> > > > +/* Trusted arguments are those which are meant to be referenced
> > arguments
> > > > with
> > > > + * unchanged offset. It is used to enforce that pointers obtained from
> > acquire
> > > > + * kfuncs remain unmodified when being passed to helpers taking trusted
> > args.
> > > > + *
> > > > + * Consider
> > > > + *   struct foo {
> > > > + *           int data;
> > > > + *           struct foo *next;
> > > > + *   };
> > > > + *
> > > > + *   struct bar {
> > > > + *           int data;
> > > > + *           struct foo f;
> > > > + *   };
> > > > + *
> > > > + *   struct foo *f = alloc_foo(); // Acquire kfunc
> > > > + *   struct bar *b = alloc_bar(); // Acquire kfunc
> > > > + *
> > > > + * If a kfunc set_foo_data() wants to operate only on the allocated object,
> > it
> > > > + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
> > > > + *
> > > > + *   set_foo_data(f, 42);       // Allowed
> > > > + *   set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> > > > + *   set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> > > > + *   set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
> > > > + *
> > > > + * In the final case, usually for the purposes of type matching, it is deduced
> > > > + * by looking at the type of the member at the offset, but due to the
> > > > + * requirement of trusted argument, this deduction will be strict and not
> > done
> > > > + * for this case.
> > > > + */
> > > > +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > > > arguments */
> > >
> > > Hi Kumar
> > >
> > > would it make sense to introduce per-parameter flags? I have a function
> > > that has several parameters, but only one is referenced.
> > >
> >
> > I have a patch for that in my local branch, I can fix it up and post
> > it. But first, can you give an example of where you think you need it?
>
> I have pushed the complete patch set here, for testing:
>
> https://github.com/robertosassu/vmtest/tree/bpf-verify-sig-v9/travis-ci/diffs
>
> I rebased to bpf-next/master, and introduced KF_SLEEPABLE (similar
> functionality of " btf: Add a new kfunc set which allows to mark
> a function to be sleepable" from Benjamin Tissoires).
>
> The patch where I would use per-parameter KF_TRUSTED_ARGS is
> number 8. I also used your new API in patch 7 and it works well.
>

Ok, looks like you'll need it for the struct key * argument as there
are multiple pointers in the argument list and not all of them need to
be trusted. I will clean up and post the patch with a test later today
to the list.

> I didn't repost, as I'm waiting for comments on v8.
>
> Thanks
>
> Roberto
Roberto Sassu July 26, 2022, 12:58 p.m. UTC | #7
> From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> Sent: Tuesday, July 26, 2022 2:56 PM
> On Tue, 26 Jul 2022 at 12:02, Roberto Sassu <roberto.sassu@huawei.com>
> wrote:
> >
> > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > Sent: Tuesday, July 26, 2022 11:30 AM
> > > On Mon, 25 Jul 2022 at 11:52, Roberto Sassu <roberto.sassu@huawei.com>
> > > wrote:
> > > >
> > > > > From: Kumar Kartikeya Dwivedi [mailto:memxor@gmail.com]
> > > > > Sent: Thursday, July 21, 2022 3:43 PM
> > > > > Teach the verifier to detect a new KF_TRUSTED_ARGS kfunc flag, which
> > > > > means each pointer argument must be trusted, which we define as a
> > > > > pointer that is referenced (has non-zero ref_obj_id) and also needs to
> > > > > have its offset unchanged, similar to how release functions expect their
> > > > > argument. This allows a kfunc to receive pointer arguments unchanged
> > > > > from the result of the acquire kfunc.
> > > > >
> > > > > This is required to ensure that kfunc that operate on some object only
> > > > > work on acquired pointers and not normal PTR_TO_BTF_ID with same
> type
> > > > > which can be obtained by pointer walking. The restrictions applied to
> > > > > release arguments also apply to trusted arguments. This implies that
> > > > > strict type matching (not deducing type by recursively following members
> > > > > at offset) and OBJ_RELEASE offset checks (ensuring they are zero) are
> > > > > used for trusted pointer arguments.
> > > > >
> > > > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > > > ---
> > > > >  include/linux/btf.h | 32 ++++++++++++++++++++++++++++++++
> > > > >  kernel/bpf/btf.c    | 17 ++++++++++++++---
> > > > >  net/bpf/test_run.c  |  5 +++++
> > > > >  3 files changed, 51 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > > > > index 6dfc6eaf7f8c..cb63aa71e82f 100644
> > > > > --- a/include/linux/btf.h
> > > > > +++ b/include/linux/btf.h
> > > > > @@ -17,6 +17,38 @@
> > > > >  #define KF_RELEASE   (1 << 1) /* kfunc is a release function */
> > > > >  #define KF_RET_NULL  (1 << 2) /* kfunc returns a pointer that may be
> NULL
> > > */
> > > > >  #define KF_KPTR_GET  (1 << 3) /* kfunc returns reference to a kptr */
> > > > > +/* Trusted arguments are those which are meant to be referenced
> > > arguments
> > > > > with
> > > > > + * unchanged offset. It is used to enforce that pointers obtained from
> > > acquire
> > > > > + * kfuncs remain unmodified when being passed to helpers taking
> trusted
> > > args.
> > > > > + *
> > > > > + * Consider
> > > > > + *   struct foo {
> > > > > + *           int data;
> > > > > + *           struct foo *next;
> > > > > + *   };
> > > > > + *
> > > > > + *   struct bar {
> > > > > + *           int data;
> > > > > + *           struct foo f;
> > > > > + *   };
> > > > > + *
> > > > > + *   struct foo *f = alloc_foo(); // Acquire kfunc
> > > > > + *   struct bar *b = alloc_bar(); // Acquire kfunc
> > > > > + *
> > > > > + * If a kfunc set_foo_data() wants to operate only on the allocated
> object,
> > > it
> > > > > + * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage
> like:
> > > > > + *
> > > > > + *   set_foo_data(f, 42);       // Allowed
> > > > > + *   set_foo_data(f->next, 42); // Rejected, non-referenced pointer
> > > > > + *   set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
> > > > > + *   set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
> > > > > + *
> > > > > + * In the final case, usually for the purposes of type matching, it is
> deduced
> > > > > + * by looking at the type of the member at the offset, but due to the
> > > > > + * requirement of trusted argument, this deduction will be strict and not
> > > done
> > > > > + * for this case.
> > > > > + */
> > > > > +#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer
> > > > > arguments */
> > > >
> > > > Hi Kumar
> > > >
> > > > would it make sense to introduce per-parameter flags? I have a function
> > > > that has several parameters, but only one is referenced.
> > > >
> > >
> > > I have a patch for that in my local branch, I can fix it up and post
> > > it. But first, can you give an example of where you think you need it?
> >
> > I have pushed the complete patch set here, for testing:
> >
> > https://github.com/robertosassu/vmtest/tree/bpf-verify-sig-v9/travis-ci/diffs
> >
> > I rebased to bpf-next/master, and introduced KF_SLEEPABLE (similar
> > functionality of " btf: Add a new kfunc set which allows to mark
> > a function to be sleepable" from Benjamin Tissoires).
> >
> > The patch where I would use per-parameter KF_TRUSTED_ARGS is
> > number 8. I also used your new API in patch 7 and it works well.
> >
> 
> Ok, looks like you'll need it for the struct key * argument as there
> are multiple pointers in the argument list and not all of them need to
> be trusted. I will clean up and post the patch with a test later today
> to the list.

Yes, thanks a lot!

Roberto
diff mbox series

Patch

diff --git a/include/linux/btf.h b/include/linux/btf.h
index 6dfc6eaf7f8c..cb63aa71e82f 100644
--- a/include/linux/btf.h
+++ b/include/linux/btf.h
@@ -17,6 +17,38 @@ 
 #define KF_RELEASE	(1 << 1) /* kfunc is a release function */
 #define KF_RET_NULL	(1 << 2) /* kfunc returns a pointer that may be NULL */
 #define KF_KPTR_GET	(1 << 3) /* kfunc returns reference to a kptr */
+/* Trusted arguments are those which are meant to be referenced arguments with
+ * unchanged offset. It is used to enforce that pointers obtained from acquire
+ * kfuncs remain unmodified when being passed to helpers taking trusted args.
+ *
+ * Consider
+ *	struct foo {
+ *		int data;
+ *		struct foo *next;
+ *	};
+ *
+ *	struct bar {
+ *		int data;
+ *		struct foo f;
+ *	};
+ *
+ *	struct foo *f = alloc_foo(); // Acquire kfunc
+ *	struct bar *b = alloc_bar(); // Acquire kfunc
+ *
+ * If a kfunc set_foo_data() wants to operate only on the allocated object, it
+ * will set the KF_TRUSTED_ARGS flag, which will prevent unsafe usage like:
+ *
+ *	set_foo_data(f, 42);	   // Allowed
+ *	set_foo_data(f->next, 42); // Rejected, non-referenced pointer
+ *	set_foo_data(&f->next, 42);// Rejected, referenced, but bad offset
+ *	set_foo_data(&b->f, 42);   // Rejected, referenced, but wrong type
+ *
+ * In the final case, usually for the purposes of type matching, it is deduced
+ * by looking at the type of the member at the offset, but due to the
+ * requirement of trusted argument, this deduction will be strict and not done
+ * for this case.
+ */
+#define KF_TRUSTED_ARGS (1 << 4) /* kfunc only takes trusted pointer arguments */
 
 struct btf;
 struct btf_member;
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index 4d9c2d88720f..7ac971ea98d1 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6174,10 +6174,10 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 				    u32 kfunc_flags)
 {
 	enum bpf_prog_type prog_type = resolve_prog_type(env->prog);
+	bool rel = false, kptr_get = false, trusted_arg = false;
 	struct bpf_verifier_log *log = &env->log;
 	u32 i, nargs, ref_id, ref_obj_id = 0;
 	bool is_kfunc = btf_is_kernel(btf);
-	bool rel = false, kptr_get = false;
 	const char *func_name, *ref_tname;
 	const struct btf_type *t, *ref_t;
 	const struct btf_param *args;
@@ -6211,6 +6211,7 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 		/* Only kfunc can be release func */
 		rel = kfunc_flags & KF_RELEASE;
 		kptr_get = kfunc_flags & KF_KPTR_GET;
+		trusted_arg = kfunc_flags & KF_TRUSTED_ARGS;
 	}
 
 	/* check that BTF function arguments match actual types that the
@@ -6235,10 +6236,19 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			return -EINVAL;
 		}
 
+		/* Check if argument must be a referenced pointer, args + i has
+		 * been verified to be a pointer (after skipping modifiers).
+		 */
+		if (is_kfunc && trusted_arg && !reg->ref_obj_id) {
+			bpf_log(log, "R%d must be referenced\n", regno);
+			return -EINVAL;
+		}
+
 		ref_t = btf_type_skip_modifiers(btf, t->type, &ref_id);
 		ref_tname = btf_name_by_offset(btf, ref_t->name_off);
 
-		if (rel && reg->ref_obj_id)
+		/* Trusted args have the same offset checks as release arguments */
+		if (trusted_arg || (rel && reg->ref_obj_id))
 			arg_type |= OBJ_RELEASE;
 		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
 		if (ret < 0)
@@ -6336,7 +6346,8 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env,
 			reg_ref_tname = btf_name_by_offset(reg_btf,
 							   reg_ref_t->name_off);
 			if (!btf_struct_ids_match(log, reg_btf, reg_ref_id,
-						  reg->off, btf, ref_id, rel && reg->ref_obj_id)) {
+						  reg->off, btf, ref_id,
+						  trusted_arg || (rel && reg->ref_obj_id))) {
 				bpf_log(log, "kernel function %s args#%d expected pointer to %s %s but R%d has a pointer to %s %s\n",
 					func_name, i,
 					btf_type_str(ref_t), ref_tname,
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index ca5b7234a350..cbc9cd5058cb 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -691,6 +691,10 @@  noinline void bpf_kfunc_call_test_mem_len_fail2(u64 *mem, int len)
 {
 }
 
+noinline void bpf_kfunc_call_test_ref(struct prog_test_ref_kfunc *p)
+{
+}
+
 __diag_pop();
 
 ALLOW_ERROR_INJECTION(bpf_modify_return_test, ERRNO);
@@ -714,6 +718,7 @@  BTF_ID_FLAGS(func, bpf_kfunc_call_test_fail3)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_pass1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail1)
 BTF_ID_FLAGS(func, bpf_kfunc_call_test_mem_len_fail2)
+BTF_ID_FLAGS(func, bpf_kfunc_call_test_ref, KF_TRUSTED_ARGS)
 BTF_SET8_END(test_sk_check_kfunc_ids)
 
 static void *bpf_test_init(const union bpf_attr *kattr, u32 user_size,