On Fri, Mar 4, 2022 at 7:21 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Fri, Mar 04, 2022 at 06:28:26PM +0100, Benjamin Tissoires wrote:
> > HID is a protocol that could benefit from using BPF too.
> >
> > This patch implements a net-like use of BPF capability for HID.
> > Any incoming report coming from the device can be injected into a series
> > of BPF programs that can modify it or even discard it by setting the
> > size in the context to 0.
> >
> > The kernel/bpf implementation is based on net-namespace.c, with only
> > the bpf_link part kept, there is no real points in keeping the
> > bpf_prog_{attach|detach} API.
> >
> > The implementation here is only focusing on the bpf changes. The HID
> > changes that hooks onto this are coming in a separate patch.
> >
> > Given that HID can be compiled in as a module, and the functions that
> > kernel/bpf/hid.c needs to call in hid.ko are exported in struct hid_hooks.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > ---
> >
> > changes in v2:
> > - split the series by bpf/libbpf/hid/selftests and samples
> > - unsigned long -> __u16 in uapi/linux/bpf_hid.h
> > - change the bpf_ctx to be of variable size, with a min of 1024 bytes
> > - make this 1 kB available directly from bpf program, the rest will
> > need a helper
> > - add some more doc comments in uapi
> > ---
> > include/linux/bpf-hid.h | 108 ++++++++
> > include/linux/bpf_types.h | 4 +
> > include/linux/hid.h | 5 +
> > include/uapi/linux/bpf.h | 7 +
> > include/uapi/linux/bpf_hid.h | 39 +++
> > kernel/bpf/Makefile | 3 +
> > kernel/bpf/hid.c | 437 +++++++++++++++++++++++++++++++++
> > kernel/bpf/syscall.c | 8 +
> > tools/include/uapi/linux/bpf.h | 7 +
> > 9 files changed, 618 insertions(+)
> > create mode 100644 include/linux/bpf-hid.h
> > create mode 100644 include/uapi/linux/bpf_hid.h
> > create mode 100644 kernel/bpf/hid.c
> >
> > diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
> > new file mode 100644
> > index 000000000000..3cda78051b5f
> > --- /dev/null
> > +++ b/include/linux/bpf-hid.h
> > @@ -0,0 +1,108 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _BPF_HID_H
> > +#define _BPF_HID_H
> > +
> > +#include <linux/mutex.h>
> > +#include <uapi/linux/bpf.h>
> > +#include <uapi/linux/bpf_hid.h>
> > +#include <linux/list.h>
> > +#include <linux/slab.h>
> > +
> > +struct bpf_prog;
> > +struct bpf_prog_array;
> > +struct hid_device;
> > +
> > +enum bpf_hid_attach_type {
> > + BPF_HID_ATTACH_INVALID = -1,
> > + BPF_HID_ATTACH_DEVICE_EVENT = 0,
> > + MAX_BPF_HID_ATTACH_TYPE
> > +};
> > +
> > +struct bpf_hid {
> > + struct hid_bpf_ctx *ctx;
> > +
> > + /* Array of programs to run compiled from links */
> > + struct bpf_prog_array __rcu *run_array[MAX_BPF_HID_ATTACH_TYPE];
> > + struct list_head links[MAX_BPF_HID_ATTACH_TYPE];
> > +};
> > +
> > +static inline enum bpf_hid_attach_type
> > +to_bpf_hid_attach_type(enum bpf_attach_type attach_type)
> > +{
> > + switch (attach_type) {
> > + case BPF_HID_DEVICE_EVENT:
> > + return BPF_HID_ATTACH_DEVICE_EVENT;
> > + default:
> > + return BPF_HID_ATTACH_INVALID;
> > + }
> > +}
> > +
> > +static inline struct hid_bpf_ctx *bpf_hid_allocate_ctx(struct hid_device *hdev,
> > + size_t data_size)
> > +{
> > + struct hid_bpf_ctx *ctx;
> > +
> > + /* ensure data_size is between min and max */
> > + data_size = clamp_val(data_size,
> > + HID_BPF_MIN_BUFFER_SIZE,
> > + HID_BPF_MAX_BUFFER_SIZE);
>
> Do you want to return an error if the data size is not within the range?
That was not something I was counting on.
Though I am thinking of not necessarily clamping this value in the end
because I might have found a way to not do the initial memcpy when
running a prog, and so not having to limit the size of the data.
> Otherwise people will just start to use crazy values and you will always
> be limiting them?
The users of this helper are really limited to drivers/hid/hid_pbf.c
and kernel/bpf/hid.c. And they are known in advance and there must be
only one user per attach type.
The only thing where the data might explode is when in used with
SEC(hid/device_event), because we statically allocate one bpf_ctx
based on the device report descriptor.
But if the required size is bigger than HID_BPF_MAX_BUFFER_SIZE, the
device will not probe or at least already logs something in the dmesg
that we are using a too big buffer.
>
> > +
> > + ctx = kzalloc(sizeof(*ctx) + data_size, GFP_KERNEL);
> > + if (!ctx)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + ctx->hdev = hdev;
> > + ctx->allocated_size = data_size;
> > +
> > + return ctx;
> > +}
>
> And why is this an inline function? Why not put it in a .c file?
The problem I have here is that the hid module can be loaded as an
external module. So I can not directly use that helper from hid.ko
from kernel/bpf/hid.c (I need it there once for the
SEC(hid/user_event) bprogram attach type).
So the solution would be to have the code in the c part of
kernel/bpf/hid.c and export the function as GPL, but I wanted to have
the minimum of knowledge of HID-BPF internals in that file. So I ended
up using an inline so I can reuse it independently in kernel/bpf/hid.c
and drivers/hid/hid-bpf.c.
>
> > +
> > +union bpf_attr;
> > +struct bpf_prog;
> > +
> > +#if IS_ENABLED(CONFIG_HID)
> > +int bpf_hid_prog_query(const union bpf_attr *attr,
> > + union bpf_attr __user *uattr);
> > +int bpf_hid_link_create(const union bpf_attr *attr,
> > + struct bpf_prog *prog);
> > +#else
> > +static inline int bpf_hid_prog_query(const union bpf_attr *attr,
> > + union bpf_attr __user *uattr)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > +static inline int bpf_hid_link_create(const union bpf_attr *attr,
> > + struct bpf_prog *prog)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +#endif
> > +
> > +static inline bool bpf_hid_link_empty(struct bpf_hid *bpf,
> > + enum bpf_hid_attach_type type)
> > +{
> > + return list_empty(&bpf->links[type]);
> > +}
> > +
> > +struct bpf_hid_hooks {
> > + struct hid_device *(*hdev_from_fd)(int fd);
> > + int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > + void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> > +};
> > +
> > +#ifdef CONFIG_BPF
> > +int bpf_hid_init(struct hid_device *hdev);
> > +void bpf_hid_exit(struct hid_device *hdev);
> > +void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks);
> > +#else
> > +static inline int bpf_hid_init(struct hid_device *hdev)
> > +{
> > + return 0;
> > +}
> > +
> > +static inline void bpf_hid_exit(struct hid_device *hdev) {}
> > +static inline void bpf_hid_set_hooks(struct bpf_hid_hooks *hooks) {}
> > +#endif
> > +
> > +#endif /* _BPF_HID_H */
> > diff --git a/include/linux/bpf_types.h b/include/linux/bpf_types.h
> > index 48a91c51c015..1509862aacc4 100644
> > --- a/include/linux/bpf_types.h
> > +++ b/include/linux/bpf_types.h
> > @@ -76,6 +76,10 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_EXT, bpf_extension,
> > BPF_PROG_TYPE(BPF_PROG_TYPE_LSM, lsm,
> > void *, void *)
> > #endif /* CONFIG_BPF_LSM */
> > +#if IS_ENABLED(CONFIG_HID)
> > +BPF_PROG_TYPE(BPF_PROG_TYPE_HID, hid,
> > + __u32, u32)
>
> Why the mix of __u32 and u32 here?
This is actually valid. I tracked it down to kernel/bpf/btf.c with:
static union {
struct bpf_ctx_convert {
#define BPF_PROG_TYPE(_id, _name, prog_ctx_type, kern_ctx_type) \
prog_ctx_type _id##_prog; \
kern_ctx_type _id##_kern;
#include <linux/bpf_types.h>
#undef BPF_PROG_TYPE
} *__t;
/* 't' is written once under lock. Read many times. */
const struct btf_type *t;
} bpf_ctx_convert;
So prog_ctx_type represents a user API, while kern_ctx_type
represents the kernel counterpart.
That being said, this is plain wrong, because I am not using u32 as
bpf context, but a properly defined struct :o)
So I probably need to amend this to be either "void *, void *)" or
something better (I'll ask Song in my reply to him).
>
> > +#endif
> > #endif
> > BPF_PROG_TYPE(BPF_PROG_TYPE_SYSCALL, bpf_syscall,
> > void *, void *)
> > diff --git a/include/linux/hid.h b/include/linux/hid.h
> > index 7487b0586fe6..56f6f4ad45a7 100644
> > --- a/include/linux/hid.h
> > +++ b/include/linux/hid.h
> > @@ -15,6 +15,7 @@
> >
> >
> > #include <linux/bitops.h>
> > +#include <linux/bpf-hid.h>
> > #include <linux/types.h>
> > #include <linux/slab.h>
> > #include <linux/list.h>
> > @@ -639,6 +640,10 @@ struct hid_device { /* device report descriptor */
> > struct list_head debug_list;
> > spinlock_t debug_list_lock;
> > wait_queue_head_t debug_wait;
> > +
> > +#ifdef CONFIG_BPF
> > + struct bpf_hid bpf;
> > +#endif
> > };
> >
> > #define to_hid_device(pdev) \
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index afe3d0d7f5f2..5978b92cacd3 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -952,6 +952,7 @@ enum bpf_prog_type {
> > BPF_PROG_TYPE_LSM,
> > BPF_PROG_TYPE_SK_LOOKUP,
> > BPF_PROG_TYPE_SYSCALL, /* a program that can execute syscalls */
> > + BPF_PROG_TYPE_HID,
> > };
> >
> > enum bpf_attach_type {
> > @@ -997,6 +998,7 @@ enum bpf_attach_type {
> > BPF_SK_REUSEPORT_SELECT,
> > BPF_SK_REUSEPORT_SELECT_OR_MIGRATE,
> > BPF_PERF_EVENT,
> > + BPF_HID_DEVICE_EVENT,
> > __MAX_BPF_ATTACH_TYPE
> > };
> >
> > @@ -1011,6 +1013,7 @@ enum bpf_link_type {
> > BPF_LINK_TYPE_NETNS = 5,
> > BPF_LINK_TYPE_XDP = 6,
> > BPF_LINK_TYPE_PERF_EVENT = 7,
> > + BPF_LINK_TYPE_HID = 8,
> >
> > MAX_BPF_LINK_TYPE,
> > };
> > @@ -5870,6 +5873,10 @@ struct bpf_link_info {
> > struct {
> > __u32 ifindex;
> > } xdp;
> > + struct {
> > + __s32 hidraw_ino;
>
> "ino"? We have lots of letters to spell words out :)
no comments... :)
>
> > + __u32 attach_type;
> > + } hid;
> > };
> > } __attribute__((aligned(8)));
> >
> > diff --git a/include/uapi/linux/bpf_hid.h b/include/uapi/linux/bpf_hid.h
> > new file mode 100644
> > index 000000000000..975ca5bd526f
> > --- /dev/null
> > +++ b/include/uapi/linux/bpf_hid.h
> > @@ -0,0 +1,39 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later WITH Linux-syscall-note */
> > +
> > +/*
> > + * HID BPF public headers
> > + *
> > + * Copyright (c) 2021 Benjamin Tissoires
>
> Did you forget the copyright line on the other .h file above?
oops
>
> > + */
> > +
> > +#ifndef _UAPI__LINUX_BPF_HID_H__
> > +#define _UAPI__LINUX_BPF_HID_H__
> > +
> > +#include <linux/types.h>
> > +
> > +/*
> > + * The first 1024 bytes are available directly in the bpf programs.
> > + * To access the rest of the data (if allocated_size is bigger
> > + * than 1024, you need to use bpf_hid_ helpers.
> > + */
> > +#define HID_BPF_MIN_BUFFER_SIZE 1024
> > +#define HID_BPF_MAX_BUFFER_SIZE 16384 /* in sync with HID_MAX_BUFFER_SIZE */
>
> Can't you just use HID_MAX_BUFFER_SIZE?
Call me dumb, but curiously I could not get my code to compile there.
If I include linux/hid.h, things are getting messy and either the
tools or the kernel itself was not compiling properly (couldn't really
remember what was failing exactly, sorry).
>
> Anyway, all minor stuff, looks good!
Thanks. Not sure I'll keep the bpf_ctx the same after further
thoughts, but I appreciate the review :)
Cheers,
Benjamin
>
> Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>
