Re: [PATCH 2/3] HID: bpf: fix __hid_bpf_hw_check_params report length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] The logic to reserve an extra byte for the report ID incorrectly checks `report_enum->numbered == 0` instead of `report->id == 0`, leading to buffer truncation for unnumbered reports on devices that also declare numbered reports.

Pre-existing issues:
- [High] An integer sign mismatch in `hid_bpf_hw_request` causes negative error codes returned by the transport driver to be cast to `size_t` and evaluated as large positive values, incorrectly converting hardware errors into a 'success' return value.
--

commit 2abe2f741986f5f3130a3dadda6bf82df9cbea21
Author: Benjamin Tissoires <bentiss@kernel.org>

HID: bpf: fix __hid_bpf_hw_check_params report length
    
This patch modifies __hid_bpf_hw_check_params to reserve an extra byte for
the report ID during hardware requests on unnumbered reports. This ensures
transport drivers do not strip the first byte of the payload and reject the
request due to insufficient buffer size.

> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index 536f6d01fd14c..b1de1dd0f21d0 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
[ ... ]
> @@ -388,6 +388,10 @@ __hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
>  
>  	report_len = hid_report_len(report);
>  
> +	/* unnumbered reports need to have a report ID reserved in the first byte */
> +	if (hw_request && report_enum->numbered == 0)
> +		report_len += 1;
> +

[Severity: Medium]
Does this correctly handle devices that declare both unnumbered and numbered
reports?

If a non-compliant HID device declares an unnumbered report where the report
ID is 0, but also declares other numbered reports, report_enum->numbered
will be set to 1.

When a BPF program attempts a hardware request for the unnumbered report on
such a device, the report_enum->numbered == 0 check will evaluate to false
and skip the +1 padding. The BPF-provided buffer size is then clamped, but
the transport driver will still strip the first byte before transmission,
leading to buffer truncation.

Would it be safer to check if report->id == 0 instead?

>  	if (*buf__sz > report_len)
>  		*buf__sz = report_len;
>  
[ ... ]
> @@ -420,7 +424,7 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
>  		return -EDEADLOCK;
>  
>  	/* check arguments */
> -	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
> +	ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype, true);
>  	if (ret)
>  		return ret;
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential integer sign mismatch
in hid_bpf_hw_request involving the size variable passed here.

In hid_bpf_hw_request, size is defined as a size_t, while ret is an int.
Later in the function, after hid_hw_raw_request returns its status into ret,
the code checks:

    if (ret > size)
        ret = size;
    if (ret > 0)
        memcpy(buf, dma_data, ret);

If a transport driver encounters a hardware error and returns a negative
error code such as -EPIPE, ret is implicitly promoted to size_t when compared
against size. This causes the negative error code to evaluate as a large
positive number, making the (ret > size) condition true.

This silently converts the hardware error into a success return value and
copies the unmodified buffer back, which could leave BPF programs operating
on uninitialized or stale data.

Can this be addressed by casting size to an ssize_t during the comparison, or
by handling negative values in ret before the comparison?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-wip-bpf-check_report-v1-0-efe9a2a1ed28@kernel.org?part=2