linux-kernel - Re: [PATCH] hid: bpf: avoid building struct ops without JIT

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <gf7t6iyj3ueewvbbmqo2ypzitiy6bvnzj2l6tgccvi22xe5fgm@xvlbq3vkndgr>
Date: Fri, 19 Jul 2024 15:52:31 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Arnd Bergmann <arnd@...nel.org>
Cc: Jiri Kosina <jikos@...nel.org>, Alexei Starovoitov <ast@...nel.org>, 
	Arnd Bergmann <arnd@...db.de>, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org, 
	bpf@...r.kernel.org
Subject: Re: [PATCH] hid: bpf: avoid building struct ops without JIT

On Jul 19 2024, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
> 
> The module does not do anything when the JIT is disabled, but instead
> causes a warning:
> 
> In file included from include/linux/bpf_verifier.h:7,
>                  from drivers/hid/bpf/hid_bpf_struct_ops.c:10:
> drivers/hid/bpf/hid_bpf_struct_ops.c: In function 'hid_bpf_struct_ops_init':
> include/linux/bpf.h:1853:50: error: statement with no effect [-Werror=unused-value]
>  1853 | #define register_bpf_struct_ops(st_ops, type) ({ (void *)(st_ops); 0; })
>       |                                                  ^~~~~~~~~~~~~~~~
> drivers/hid/bpf/hid_bpf_struct_ops.c:305:16: note: in expansion of macro 'register_bpf_struct_ops'
>   305 |         return register_bpf_struct_ops(&bpf_hid_bpf_ops, hid_bpf_ops);
>       |                ^~~~~~~~~~~~~~~~~~~~~~~
> 
> This could be avoided by making HID-BPF just depend on JIT, but that
> is probably not what we want here. Checking the other users of struct_ops,
> I see that those just leave out the struct_ops usage, so do the same here.

Actually, if we make the struct_ops part only depend on JIT HID-BPF is
kind of moot. All we could do is use HID-BPF to communicate with the
device, without getting any feedback, so nothing much more than what
hidraw provides.

The only "interesting" bit we could do is inject a new event on a device
as if it were originated from the device itself, but I really do not see
the point without the struct_ops hooks.

So I think struct_ops is now the base for HID-BPF, and if it's not
available, we should not have HID-BPF at all.

> 
> Fixes: ebc0d8093e8c ("HID: bpf: implement HID-BPF through bpf_struct_ops")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
>  drivers/hid/bpf/Makefile           | 3 ++-
>  drivers/hid/bpf/hid_bpf_dispatch.h | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/bpf/Makefile b/drivers/hid/bpf/Makefile
> index d1f2b81788ca..7566be8eefba 100644
> --- a/drivers/hid/bpf/Makefile
> +++ b/drivers/hid/bpf/Makefile
> @@ -8,4 +8,5 @@ LIBBPF_INCLUDE = $(srctree)/tools/lib
>  obj-$(CONFIG_HID_BPF) += hid_bpf.o
>  CFLAGS_hid_bpf_dispatch.o += -I$(LIBBPF_INCLUDE)
>  CFLAGS_hid_bpf_jmp_table.o += -I$(LIBBPF_INCLUDE)

Unrelated to your patch, but looks like I forgot to remove that entry
from the Makefile now that hid_bpf_jmp_table.c is not available :)

Cheers,
Benjamin

> -hid_bpf-objs += hid_bpf_dispatch.o hid_bpf_struct_ops.o
> +hid_bpf-y += hid_bpf_dispatch.o
> +hid_bpf-$(CONFIG_BPF_JIT) += hid_bpf_struct_ops.o
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.h b/drivers/hid/bpf/hid_bpf_dispatch.h
> index 44c6ea22233f..577572f41454 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.h
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.h
> @@ -14,7 +14,13 @@ struct hid_bpf_ctx_kern {
>  struct hid_device *hid_get_device(unsigned int hid_id);
>  void hid_put_device(struct hid_device *hid);
>  int hid_bpf_allocate_event_data(struct hid_device *hdev);
> +#ifdef CONFIG_BPF_JIT
>  void __hid_bpf_ops_destroy_device(struct hid_device *hdev);
> +#else
> +static inline void __hid_bpf_ops_destroy_device(struct hid_device *hdev)
> +{
> +}
> +#endif
>  int hid_bpf_reconnect(struct hid_device *hdev);
>  
>  struct bpf_prog;
> -- 
> 2.39.2
>