[RFC v3 1/2] HID: core: Mitigate potential OOB by removing bogus memset()

[RFC v3 1/2] HID: core: Mitigate potential OOB by removing bogus memset()

[Lee Jones]

The memset() in hid_report_raw_event() has the good intention of
clearing out bogus data by zeroing the area from the end of the incoming
data string to the assumed end of the buffer.  However, as we have
previously seen, doing so can easily result in OOB reads and writes in
the subsequent thread of execution.

The current suggestion from one of the HID maintainers is to remove the
memset() and simply return if the incoming event buffer size is not
large enough to fill the associated report.

Suggested-by Benjamin Tissoires <bentiss@kernel.org>
Signed-off-by: Lee Jones <lee@kernel.org>
---
v2 -> v3: Instead of removing the check entirely, show a warning and return early

RFC query: Is it better to return SUCCESS or -EINVAL?

 drivers/hid/hid-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index a5b3a8ca2fcb..da9231ca42bc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2057,9 +2057,9 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
 		rsize = max_buffer_size;
 
 	if (csize < rsize) {
-		dbg_hid("report %d is too short, (%d < %d)\n", report->id,
-				csize, rsize);
-		memset(cdata + csize, 0, rsize - csize);
+		hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %d)\n",
+				     report->id, rsize, csize);
+		goto out;
 	}
 
 	if ((hid->claimed & HID_CLAIMED_HIDDEV) && hid->hiddev_report_event)
-- 
2.53.0.473.g4a7958ca14-goog

[RFC v3 2/2] HID: core: Check to ensure report responses match the request

[Lee Jones]

It is possible for a malicious (or clumsy) device to respond to a
specific report's feature request using a completely different report
ID.  This can cause confusion in the HID core resulting in nasty
side-effects such as OOB writes.

Add a check to ensure that the report ID in the response, matches the
one that was requested.

Signed-off-by: Lee Jones <lee@kernel.org>
---
v2 -> v3: Cover more bases by moving the check up a layer from MT to HID Core

RFC query: Is this always okay?
           Should the report number always match the request?
	   Are there legitimate times where the two would differ?

 drivers/hid/hid-core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index da9231ca42bc..da4078554331 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2490,8 +2490,17 @@ int __hid_hw_raw_request(struct hid_device *hdev,
 	if (ret)
 		return ret;
 
-	return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
-					    rtype, reqtype);
+	ret = hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
+					   rtype, reqtype);
+	if (ret)
+		return ret;
+
+	if (reportnum != buf[0]) {
+		hid_err(hdev, "Returned feature report did not match the request\n");
+		return -EINVAL;
+	}
+
+	return 0;
 }
 
 /**
-- 
2.53.0.473.g4a7958ca14-goog

Re: [RFC v3 2/2] HID: core: Check to ensure report responses match the request

[Benjamin Tissoires]

On Mar 09 2026, Lee Jones wrote:
> It is possible for a malicious (or clumsy) device to respond to a
> specific report's feature request using a completely different report
> ID.  This can cause confusion in the HID core resulting in nasty
> side-effects such as OOB writes.
> 
> Add a check to ensure that the report ID in the response, matches the
> one that was requested.
> 
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> v2 -> v3: Cover more bases by moving the check up a layer from MT to HID Core
> 
> RFC query: Is this always okay?
>            Should the report number always match the request?
> 	   Are there legitimate times where the two would differ?

Technically, there is no reasons for a HID_SET_REPORT request to change
the incoming buffer. So that test might break it.

I prefered fixing the calling sites (hid-multitouch and others), because
here we are making decisions on the device behaviour which is not ours
to make. More specifically, such a test will prevent us to fix a bogus
device by plainly rejecting the call after the facts.

Cheers,
Benjamin

> 
>  drivers/hid/hid-core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index da9231ca42bc..da4078554331 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2490,8 +2490,17 @@ int __hid_hw_raw_request(struct hid_device *hdev,
>  	if (ret)
>  		return ret;
>  
> -	return hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
> -					    rtype, reqtype);
> +	ret = hdev->ll_driver->raw_request(hdev, reportnum, buf, len,
> +					   rtype, reqtype);
> +	if (ret)
> +		return ret;
> +
> +	if (reportnum != buf[0]) {
> +		hid_err(hdev, "Returned feature report did not match the request\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  
>  /**
> -- 
> 2.53.0.473.g4a7958ca14-goog
> 
> 

Re: [RFC v3 2/2] HID: core: Check to ensure report responses match the request

[Lee Jones]

On Mon, 16 Mar 2026, Benjamin Tissoires wrote:

> On Mar 09 2026, Lee Jones wrote:
> > It is possible for a malicious (or clumsy) device to respond to a
> > specific report's feature request using a completely different report
> > ID.  This can cause confusion in the HID core resulting in nasty
> > side-effects such as OOB writes.
> > 
> > Add a check to ensure that the report ID in the response, matches the
> > one that was requested.
> > 
> > Signed-off-by: Lee Jones <lee@kernel.org>
> > ---
> > v2 -> v3: Cover more bases by moving the check up a layer from MT to HID Core
> > 
> > RFC query: Is this always okay?
> >            Should the report number always match the request?
> > 	   Are there legitimate times where the two would differ?
> 
> Technically, there is no reasons for a HID_SET_REPORT request to change
> the incoming buffer. So that test might break it.
> 
> I prefered fixing the calling sites (hid-multitouch and others), because
> here we are making decisions on the device behaviour which is not ours
> to make. More specifically, such a test will prevent us to fix a bogus
> device by plainly rejecting the call after the facts.

Okay, so this one is a NACK?  No changes, do not resend?

-- 
Lee Jones [李琼斯]

Re: [RFC v3 2/2] HID: core: Check to ensure report responses match the request

[Benjamin Tissoires]

On Mar 17 2026, Lee Jones wrote:
> On Mon, 16 Mar 2026, Benjamin Tissoires wrote:
> 
> > On Mar 09 2026, Lee Jones wrote:
> > > It is possible for a malicious (or clumsy) device to respond to a
> > > specific report's feature request using a completely different report
> > > ID.  This can cause confusion in the HID core resulting in nasty
> > > side-effects such as OOB writes.
> > > 
> > > Add a check to ensure that the report ID in the response, matches the
> > > one that was requested.
> > > 
> > > Signed-off-by: Lee Jones <lee@kernel.org>
> > > ---
> > > v2 -> v3: Cover more bases by moving the check up a layer from MT to HID Core
> > > 
> > > RFC query: Is this always okay?
> > >            Should the report number always match the request?
> > > 	   Are there legitimate times where the two would differ?
> > 
> > Technically, there is no reasons for a HID_SET_REPORT request to change
> > the incoming buffer. So that test might break it.
> > 
> > I prefered fixing the calling sites (hid-multitouch and others), because
> > here we are making decisions on the device behaviour which is not ours
> > to make. More specifically, such a test will prevent us to fix a bogus
> > device by plainly rejecting the call after the facts.
> 
> Okay, so this one is a NACK?  No changes, do not resend?
> 

Yes, NACK on this one. I've merged the hid-multitouch one which wasn't
using the API correctly, please send a followup for the other similar
cases.

Cheers,
Benjamin

Re: [RFC v3 1/2] HID: core: Mitigate potential OOB by removing bogus memset()

[Lee Jones]

On Mon, 09 Mar 2026, Lee Jones wrote:

> The memset() in hid_report_raw_event() has the good intention of
> clearing out bogus data by zeroing the area from the end of the incoming
> data string to the assumed end of the buffer.  However, as we have
> previously seen, doing so can easily result in OOB reads and writes in
> the subsequent thread of execution.
> 
> The current suggestion from one of the HID maintainers is to remove the
> memset() and simply return if the incoming event buffer size is not
> large enough to fill the associated report.
> 
> Suggested-by Benjamin Tissoires <bentiss@kernel.org>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> v2 -> v3: Instead of removing the check entirely, show a warning and return early
> 
> RFC query: Is it better to return SUCCESS or -EINVAL?
> 
>  drivers/hid/hid-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index a5b3a8ca2fcb..da9231ca42bc 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2057,9 +2057,9 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
>  		rsize = max_buffer_size;
>  
>  	if (csize < rsize) {
> -		dbg_hid("report %d is too short, (%d < %d)\n", report->id,
> -				csize, rsize);
> -		memset(cdata + csize, 0, rsize - csize);
> +		hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %d)\n",
> +				     report->id, rsize, csize);
> +		goto out;
>  	}
>  
>  	if ((hid->claimed & HID_CLAIMED_HIDDEV) && hid->hiddev_report_event)

Same here.  Are these still in your queue or would you like me to [RESEND]?

-- 
Lee Jones [李琼斯]

Re: [RFC v3 1/2] HID: core: Mitigate potential OOB by removing bogus memset()

[Benjamin Tissoires]

On Mar 09 2026, Lee Jones wrote:
> The memset() in hid_report_raw_event() has the good intention of
> clearing out bogus data by zeroing the area from the end of the incoming
> data string to the assumed end of the buffer.  However, as we have
> previously seen, doing so can easily result in OOB reads and writes in
> the subsequent thread of execution.
> 
> The current suggestion from one of the HID maintainers is to remove the
> memset() and simply return if the incoming event buffer size is not
> large enough to fill the associated report.
> 
> Suggested-by Benjamin Tissoires <bentiss@kernel.org>
> Signed-off-by: Lee Jones <lee@kernel.org>
> ---
> v2 -> v3: Instead of removing the check entirely, show a warning and return early
> 
> RFC query: Is it better to return SUCCESS or -EINVAL?

I'd say -EINVAL is better.

Also, one brain fart I had today was that this works in 99% of the cases
because the transport layer allocates a big enough buffer.

So that means that if this function, and hid_input_report() both have
the allocated size as parameter, we could make a smarter decision and
do the memset when we have enough space.

This would require an API change, and probably to keep things under
control adding a new API with the buffer_allocated_size that would be
used by uhid, usbhid, i2c-hid, and others when we can.

But let's go with the current state of the patch, and say sorry if we
break some devices later on. (patch is currently in my queue, no need to
resend it).

Cheers,
Benjamin

> 
>  drivers/hid/hid-core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index a5b3a8ca2fcb..da9231ca42bc 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2057,9 +2057,9 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
>  		rsize = max_buffer_size;
>  
>  	if (csize < rsize) {
> -		dbg_hid("report %d is too short, (%d < %d)\n", report->id,
> -				csize, rsize);
> -		memset(cdata + csize, 0, rsize - csize);
> +		hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %d)\n",
> +				     report->id, rsize, csize);
> +		goto out;
>  	}
>  
>  	if ((hid->claimed & HID_CLAIMED_HIDDEV) && hid->hiddev_report_event)
> -- 
> 2.53.0.473.g4a7958ca14-goog
> 
> 

Re: (subset) [RFC v3 1/2] HID: core: Mitigate potential OOB by removing bogus memset()

[Benjamin Tissoires]

On Mon, 09 Mar 2026 14:59:29 +0000, Lee Jones wrote:
> The memset() in hid_report_raw_event() has the good intention of
> clearing out bogus data by zeroing the area from the end of the incoming
> data string to the assumed end of the buffer.  However, as we have
> previously seen, doing so can easily result in OOB reads and writes in
> the subsequent thread of execution.
> 
> The current suggestion from one of the HID maintainers is to remove the
> memset() and simply return if the incoming event buffer size is not
> large enough to fill the associated report.
> 
> [...]

Applied, thanks!

[1/2] HID: core: Mitigate potential OOB by removing bogus memset()
      commit: 0a3fe972a7cb1404f693d6f1711f32bc1d244b1c

Best regards,
-- 
Benjamin Tissoires <bentiss@kernel.org>