Re: [PATCH 1/9] Bluetooth: HIDP: Fix GET_REPORT and SET_REPORT for unnumbered reports

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] bluetooth: hidp: skb memory leak on timeout in hidp_get_raw_report()
--

commit 9c135d466dcba28a7f3598e00e865c9ade48ff0c
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Bluetooth: HIDP: Fix GET_REPORT and SET_REPORT for unnumbered reports

Adjust Bluetooth HIDP transport callbacks to adhere to the kernel HID
subsystem convention of using report ID 0 for unnumbered reports. When
fetching or setting an unnumbered report, the report ID is appended or
stripped to ensure correct lengths and formatting over the control channel.

> diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
> index f5bdf9f1ca638..004a8eed51fb1 100644
> --- a/net/bluetooth/hidp/core.c
> +++ b/net/bluetooth/hidp/core.c
[ ... ]
> @@ -302,11 +302,25 @@ static int hidp_get_raw_report(struct hid_device *hid,
>  		}
>  	}
>  

[Severity: Medium]
Does this code leak the sk_buff if a timeout or signal occurs concurrently
with the device response?

If wait_event_interruptible_timeout() in hidp_get_raw_report() times out or is
interrupted by a signal, it proceeds to the err label, which clears the
HIDP_WAITING_FOR_RETURN flag and unlocks the mutex without freeing
session->report_return.

Concurrently, if the device response arrives and is processed by
hidp_recv_ctrl_frame(), it can set session->report_return right as the timeout
occurs:

hidp_recv_ctrl_frame() {
        ...
        /* hidp_get_raw_report() is waiting on this report. */
        session->report_return = skb;
        done_with_skb = 0;
        clear_bit(HIDP_WAITING_FOR_RETURN, &session->flags);
        wake_up_interruptible(&session->report_queue);
}

Because the error path does not check or free session->report_return, the
sk_buff is permanently leaked as subsequent requests will overwrite
session->report_return without freeing the old one.

> +	/*
> +	 * In case of unnumbered reports the response from the device will
> +	 * not have the report ID that the upper layers expect, so we need
> +	 * to stash it in the buffer ourselves and adjust the data size.
> +	 */
> +	if (!report_number) {
> +		data[0] = 0;
> +		data++;
> +		count--;
> +	}
> +
>  	skb = session->report_return;
>  	if (skb) {
> -		len = skb->len < count ? skb->len : count;
> +		len = min_t(size_t, skb->len, count);
>  		memcpy(data, skb->data, len);
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-hid-core-report-validation-v1-0-e0dd5cb60b2e@gmail.com?part=1