netdev - Re: [PATCH bpf-next v2 12/28] bpf/hid: add hid_{get|set}

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]

Date:   Fri, 4 Mar 2022 19:41:09 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Dave Marchevsky <davemarchevsky@...com>,
        Joe Stringer <joe@...ium.io>,
        Tero Kristo <tero.kristo@...ux.intel.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        netdev@...r.kernel.org, bpf@...r.kernel.org,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 12/28] bpf/hid: add hid_{get|set}_data helpers

On Fri, Mar 04, 2022 at 06:28:36PM +0100, Benjamin Tissoires wrote:
> When we process an incoming HID report, it is common to have to account
> for fields that are not aligned in the report. HID is using 2 helpers
> hid_field_extract() and implement() to pick up any data at any offset
> within the report.
> 
> Export those 2 helpers in BPF programs so users can also rely on them.
> The second net worth advantage of those helpers is that now we can
> fetch data anywhere in the report without knowing at compile time the
> location of it. The boundary checks are done in hid-bpf.c, to prevent
> a memory leak.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> 
> ---
> 
> changes in v2:
> - split the patch with libbpf and HID left outside.
> ---
>  include/linux/bpf-hid.h        |  4 +++
>  include/uapi/linux/bpf.h       | 32 ++++++++++++++++++++
>  kernel/bpf/hid.c               | 53 ++++++++++++++++++++++++++++++++++
>  tools/include/uapi/linux/bpf.h | 32 ++++++++++++++++++++
>  4 files changed, 121 insertions(+)
> 
> diff --git a/include/linux/bpf-hid.h b/include/linux/bpf-hid.h
> index 0c5000b28b20..69bb28523ceb 100644
> --- a/include/linux/bpf-hid.h
> +++ b/include/linux/bpf-hid.h
> @@ -93,6 +93,10 @@ struct bpf_hid_hooks {
>  	int (*link_attach)(struct hid_device *hdev, enum bpf_hid_attach_type type);
>  	void (*link_attached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
>  	void (*array_detached)(struct hid_device *hdev, enum bpf_hid_attach_type type);
> +	int (*hid_get_data)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> +			    u64 offset, u32 n, u8 *data, u64 data_size);
> +	int (*hid_set_data)(struct hid_device *hdev, u8 *buf, size_t buf_size,
> +			    u64 offset, u32 n, u8 *data, u64 data_size);
>  };
>  
>  #ifdef CONFIG_BPF
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index a7a8d9cfcf24..4845a20e6f96 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -5090,6 +5090,36 @@ union bpf_attr {
>   *	Return
>   *		0 on success, or a negative error in case of failure. On error
>   *		*dst* buffer is zeroed out.
> + *
> + * int bpf_hid_get_data(void *ctx, u64 offset, u32 n, u8 *data, u64 size)
> + *	Description
> + *		Get the data of size n (in bits) at the given offset (bits) in the
> + *		ctx->event.data field and store it into data.
> + *
> + *		if n is less or equal than 32, we can address with bit precision,
> + *		the value in the buffer. However, data must be a pointer to a u32
> + *		and size must be 4.
> + *
> + *		if n is greater than 32, offset and n must be a multiple of 8
> + *		and the result is working with a memcpy internally.
> + *	Return
> + *		The length of data copied into data. On error, a negative value
> + *		is returned.
> + *
> + * int bpf_hid_set_data(void *ctx, u64 offset, u32 n, u8 *data, u64 size)
> + *	Description
> + *		Set the data of size n (in bits) at the given offset (bits) in the
> + *		ctx->event.data field.
> + *
> + *		if n is less or equal than 32, we can address with bit precision,
> + *		the value in the buffer. However, data must be a pointer to a u32
> + *		and size must be 4.
> + *
> + *		if n is greater than 32, offset and n must be a multiple of 8
> + *		and the result is working with a memcpy internally.
> + *	Return
> + *		The length of data copied into ctx->event.data. On error, a negative
> + *		value is returned.

Wait, nevermind my reviewed-by previously, see my comment about how this
might be split into 4:
	bpf_hid_set_bytes()
	bpf_hid_get_bytes()
	bpf_hid_set_bits()
	bpf_hid_get_bits()

Should be easier to understand and maintain over time, right?

thanks,

greg k-h