| Message ID | 20220721134245.2450-5-memxor@gmail.com |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | New nf_conntrack kfuncs for insertion, changing timeout, status | expand |
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.
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!
> 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
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
> 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
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
> 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 --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,
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(-)