| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 9 Apr 2019 11:40:50 -0700
From: Stanislav Fomichev <sdf@...ichev.me>
To: Martin Lau <kafai@...com>
Cc: Stanislav Fomichev <sdf@...gle.com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"bpf@...r.kernel.org" <bpf@...r.kernel.org>,
"davem@...emloft.net" <davem@...emloft.net>,
"ast@...nel.org" <ast@...nel.org>,
"daniel@...earbox.net" <daniel@...earbox.net>
Subject: Re: [PATCH bpf-next v3 1/3] bpf: support input __sk_buff context in
BPF_PROG_TEST_RUN
On 04/09, Martin Lau wrote:
> On Tue, Apr 09, 2019 at 10:27:37AM -0700, Stanislav Fomichev wrote:
> > Add new set of arguments to bpf_attr for BPF_PROG_TEST_RUN:
> > * ctx_in/ctx_size_in - input context
> > * ctx_out/ctx_size_out - output context
> >
> > The intended use case is to pass some meta data to the test runs that
> > operate on skb (this has being brought up on recent LPC).
> >
> > For programs that use bpf_prog_test_run_skb, support __sk_buff input and
> > output. Initially, from input __sk_buff, copy _only_ cb and priority into
> > skb, all other non-zero fields are prohibited (with EINVAL).
> > If the user has set ctx_out/ctx_size_out, copy the potentially modified
> > __sk_buff back to the userspace.
> >
> > We require all fields of input __sk_buff except the ones we explicitly
> > support to be set to zero. The expectation is that in the future we might
> > add support for more fields and we want to fail explicitly if the user
> > runs the program on the kernel where we don't yet support them.
> >
> > The API is intentionally vague (i.e. we don't explicitly add __sk_buff
> > to bpf_attr, but ctx_in) to potentially let other test_run types use
> > this interface in the future (this can be xdp_md for xdp types for
> > example).
> >
> > v3:
> > * handle case where ctx_in is NULL, but ctx_out is not [Martin]
> > * convert size==0 checks to ptr==NULL checks and add some extra ptr
> > checks [Martin]
> >
> > v2:
> > * Addressed comments from Martin Lau
> >
> > Cc: Martin Lau <kafai@...com>
> > Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> > ---
> > include/uapi/linux/bpf.h | 7 ++
> > kernel/bpf/syscall.c | 10 ++-
> > net/bpf/test_run.c | 142 ++++++++++++++++++++++++++++++++++++---
> > 3 files changed, 150 insertions(+), 9 deletions(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 837024512baf..8e96f99cebf8 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -396,6 +396,13 @@ union bpf_attr {
> > __aligned_u64 data_out;
> > __u32 repeat;
> > __u32 duration;
> > + __u32 ctx_size_in; /* input: len of ctx_in */
> > + __u32 ctx_size_out; /* input/output: len of ctx_out
> > + * returns ENOSPC if ctx_out
> > + * is too small.
> > + */
> > + __aligned_u64 ctx_in;
> > + __aligned_u64 ctx_out;
> > } test;
> >
> > struct { /* anonymous struct used by BPF_*_GET_*_ID */
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 1d65e56594db..5bb963e8f9b0 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -1949,7 +1949,7 @@ static int bpf_prog_query(const union bpf_attr *attr,
> > return cgroup_bpf_prog_query(attr, uattr);
> > }
> >
> > -#define BPF_PROG_TEST_RUN_LAST_FIELD test.duration
> > +#define BPF_PROG_TEST_RUN_LAST_FIELD test.ctx_out
> >
> > static int bpf_prog_test_run(const union bpf_attr *attr,
> > union bpf_attr __user *uattr)
> > @@ -1962,6 +1962,14 @@ static int bpf_prog_test_run(const union bpf_attr *attr,
> > if (CHECK_ATTR(BPF_PROG_TEST_RUN))
> > return -EINVAL;
> >
> > + if ((attr->test.ctx_size_in && !attr->test.ctx_in) ||
> > + (!attr->test.ctx_size_in && attr->test.ctx_in))
> > + return -EINVAL;
> > +
> > + if ((attr->test.ctx_size_out && !attr->test.ctx_out) ||
> > + (!attr->test.ctx_size_out && attr->test.ctx_out))
> > + return -EINVAL;
> > +
> > prog = bpf_prog_get(attr->test.prog_fd);
> > if (IS_ERR(prog))
> > return PTR_ERR(prog);
> > diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> > index fab142b796ef..2023d8841c6d 100644
> > --- a/net/bpf/test_run.c
> > +++ b/net/bpf/test_run.c
> > @@ -123,12 +123,125 @@ static void *bpf_test_init(const union bpf_attr *kattr, u32 size,
> > return data;
> > }
> >
> > +static void *bpf_ctx_init(const union bpf_attr *kattr, u32 max_size)
> > +{
> > + void __user *data_in = u64_to_user_ptr(kattr->test.ctx_in);
> > + void __user *data_out = u64_to_user_ptr(kattr->test.ctx_out);
> > + u32 size = kattr->test.ctx_size_in;
> > + void *data;
> > + int err;
> > +
> > + if (!data_in && !data_out)
> > + return NULL;
> > +
> > + data = kzalloc(max_size, GFP_USER);
> > + if (!data)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + if (data_in) {
> > + err = bpf_check_uarg_tail_zero(data_in, max_size, size);
> > + if (err) {
> > + kfree(data);
> > + return ERR_PTR(err);
> > + }
> > +
> > + if (copy_from_user(data, data_in, size)) {
> A min_t is needed:
> size = min_t(u32, max_size, size);
> if (copy_from_user(data, data_in, size)) {
Ah, sorry about that, good catch! Will respin shortly.
> Others lgtm,
>
> Acked-by: Martin KaFai Lau <kafai@...com>
Thank you for a review!
> > + kfree(data);
> > + return ERR_PTR(-EFAULT);
> > + }
> > + }
> > + return data;
> > +}
Powered by blists - more mailing lists