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

[Benjamin Tissoires <bentiss@kernel.org>]

On Sep 04 2026, sashiko-bot@kernel.org wrote:
> 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.

Sashiko is wrong here. If a HID device declares a device with a report
ID of 0, the report will be tagged as numbered. The report *will* have
space for the report ID, and so there won't be any issues.

> 
> 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?

Damn, this one seems correct.

Cheers,
Benjamin

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