Re: [PATCH v2 6/8] HID: oxp: validate report size in raw_event callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: oxp: out-of-bounds read in oxp_hid_raw_event() dev_dbg()
--

commit 5b821b0394212eadced6ac6427c3ff78bbef2d73
Author: Tristan Madani <tristan@talencesecurity.com>

HID: oxp: validate report size in raw_event callbacks

This commit adds report size checks to oxp_hid_raw_event_gen_1() and
oxp_hid_raw_event_gen_2() before casting the incoming report buffer to
struct oxp_gen_1_rgb_report and struct oxp_gen_2_rgb_report.

> diff --git a/drivers/hid/hid-oxp.c b/drivers/hid/hid-oxp.c
> index 1e691ebc1199e..af9f60d45bf29 100644
> --- a/drivers/hid/hid-oxp.c
> +++ b/drivers/hid/hid-oxp.c
> @@ -318,6 +318,9 @@ static int oxp_hid_raw_event_gen_1(struct hid_device *hdev,
>  	struct led_classdev_mc *led_mc = drvdata.led_mc;
>  	struct oxp_gen_1_rgb_report *rgb_rep;
>  
> +	if (size < sizeof(struct oxp_gen_1_rgb_report))
> +		return 0;
> +
>  	if (data[1] != OXP_FID_GEN1_RGB_REPLY)
>  		return 0;
>  
> @@ -391,6 +394,9 @@ static int oxp_hid_raw_event_gen_2(struct hid_device *hdev,
>  	struct led_classdev_mc *led_mc = drvdata.led_mc;
>  	struct oxp_gen_2_rgb_report *rgb_rep;
>  
> +	if (size < sizeof(struct oxp_gen_2_rgb_report))
> +		return 0;
> +
>  	if (data[0] != OXP_FID_GEN2_STATUS_EVENT)
>  		return 0;
>  

[Severity: Medium]
Can oxp_hid_raw_event() still read past the end of data before reaching
these checks when dynamic debug or DEBUG is enabled?

In drivers/hid/hid-oxp.c, the caller oxp_hid_raw_event() logs a fixed
OXP_PACKET_SIZE (64) bytes from data before dispatching to
oxp_hid_raw_event_gen_1() or oxp_hid_raw_event_gen_2():

drivers/hid/hid-oxp.c:oxp_hid_raw_event() {
	u16 up = get_usage_page(hdev);

	dev_dbg(&hdev->dev, "raw event data: [%*ph]\n", OXP_PACKET_SIZE, data);

	switch (up) {
	case GEN1_USAGE_PAGE:
		return oxp_hid_raw_event_gen_1(hdev, report, data, size);
	case GEN2_USAGE_PAGE:
		return oxp_hid_raw_event_gen_2(hdev, report, data, size);
	...
}

When a short report arrives with size < OXP_PACKET_SIZE (such as a report
allocated for a smaller size via hid_alloc_report_buf() in __hid_request()
or injected via hid_bpf_input_report()), dev_dbg() will read 64 bytes out
of bounds from data before the new size checks in
oxp_hid_raw_event_gen_1() and oxp_hid_raw_event_gen_2() are reached.

Should dev_dbg() in oxp_hid_raw_event() pass size (or min(size,
OXP_PACKET_SIZE)) instead of OXP_PACKET_SIZE?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261018220344.1987142-1-tristmd@gmail.com?part=6