Commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow.
We only had corner cases of malicious devices exposing the OOM because in most cases, the buffer provided by the transport layer needs to be allocated at probe time and is large enough to handle all the possible reports.
However, the patch from above, which enforces the spec a little bit more introduced both regressions for devices not following the spec (not necesserally malicious), but also a stream of errors for those devices.
Let's revert to the old behavior by giving more information to HID core to be able to decide whether it can or not memset the rest of the buffer to 0 and continue the processing.
Note that the first commit makes an API change, but the callers are relatively limited, so it should be fine on its own. The second patch can't really make the same kind of API change because we have too many callers in various subsystems. We can switch them one by one to the safe approach when needed.
The last 2 patches are small cleanups I initially put together with the 2 first patches, but they can be applied on their own and don't need to be pulled in stable like the first 2.
Cheers, Benjamin
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- Changes in v3: - fixed ghib -> ghid in greybus - fixed i386 size_t debug size reported by kernel-bot - Link to v2: https://lore.kernel.org/r/20260416-wip-fix-core-v2-0-be92570e5627@kernel.org
Changes in v2: - added a small blurb explaining the difference between the safe and the non safe version of hid_safe_input_report - Link to v1: https://lore.kernel.org/r/20260415-wip-fix-core-v1-0-ed3c4c823175@kernel.org
--- Benjamin Tissoires (4): HID: pass the buffer size to hid_report_raw_event HID: core: introduce hid_safe_input_report() HID: multitouch: use __free(kfree) to clean up temporary buffers HID: wacom: use __free(kfree) to clean up temporary buffers
drivers/hid/bpf/hid_bpf_dispatch.c | 6 ++-- drivers/hid/hid-core.c | 67 ++++++++++++++++++++++++++++++-------- drivers/hid/hid-gfrm.c | 4 +-- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-multitouch.c | 18 ++++------ drivers/hid/hid-primax.c | 2 +- drivers/hid/hid-vivaldi-common.c | 2 +- drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++-- drivers/hid/usbhid/hid-core.c | 11 ++++--- drivers/hid/wacom_sys.c | 46 +++++++++----------------- drivers/staging/greybus/hid.c | 2 +- include/linux/hid.h | 6 ++-- include/linux/hid_bpf.h | 14 +++++--- 13 files changed, 109 insertions(+), 78 deletions(-) --- base-commit: 7df6572f1cb381d6b89ceed58e3b076c233c2cd0 change-id: 20260415-wip-fix-core-7d85c8516ed0
Best regards,
commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow. However, we can try to be smarter by providing both the buffer size and the data size, meaning that hid_report_raw_event() can make better decision whether we should plaining reject the buffer (buffer overflow attempt) or if we can safely memset it to 0 and pass it to the rest of the stack.
Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") Cc: stable@vger.kernel.org Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- drivers/hid/bpf/hid_bpf_dispatch.c | 6 ++++-- drivers/hid/hid-core.c | 42 +++++++++++++++++++++++++------------- drivers/hid/hid-gfrm.c | 4 ++-- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-multitouch.c | 2 +- drivers/hid/hid-primax.c | 2 +- drivers/hid/hid-vivaldi-common.c | 2 +- drivers/hid/wacom_sys.c | 6 +++--- drivers/staging/greybus/hid.c | 2 +- include/linux/hid.h | 4 ++-- include/linux/hid_bpf.h | 14 ++++++++----- 11 files changed, 53 insertions(+), 33 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c index 50c7b45c59e3..d0130658091b 100644 --- a/drivers/hid/bpf/hid_bpf_dispatch.c +++ b/drivers/hid/bpf/hid_bpf_dispatch.c @@ -24,7 +24,8 @@ EXPORT_SYMBOL(hid_ops);
u8 * dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type, u8 *data, - u32 *size, int interrupt, u64 source, bool from_bpf) + size_t *buf_size, u32 *size, int interrupt, u64 source, + bool from_bpf) { struct hid_bpf_ctx_kern ctx_kern = { .ctx = { @@ -74,6 +75,7 @@ dispatch_hid_bpf_device_event(struct hid_device *hdev, enum hid_report_type type *size = ret; }
+ *buf_size = ctx_kern.ctx.allocated_size; return ctx_kern.data; } EXPORT_SYMBOL_GPL(dispatch_hid_bpf_device_event); @@ -505,7 +507,7 @@ __hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *b if (ret) return ret;
- return hid_ops->hid_input_report(ctx->hid, type, buf, size, 0, (u64)(long)ctx, true, + return hid_ops->hid_input_report(ctx->hid, type, buf, size, size, 0, (u64)(long)ctx, true, lock_already_taken); }
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 61afec5915ec..a806820df7e5 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2033,24 +2033,32 @@ int __hid_request(struct hid_device *hid, struct hid_report *report, } EXPORT_SYMBOL_GPL(__hid_request);
-int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, - int interrupt) +int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data, + size_t bufsize, u32 size, int interrupt) { struct hid_report_enum *report_enum = hid->report_enum + type; struct hid_report *report; struct hid_driver *hdrv; int max_buffer_size = HID_MAX_BUFFER_SIZE; u32 rsize, csize = size; + size_t bsize = bufsize; u8 *cdata = data; int ret = 0;
report = hid_get_report(report_enum, data); if (!report) - goto out; + return 0; + + if (unlikely(bsize < csize)) { + hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n", + report->id, csize, bsize); + return -EINVAL; + }
if (report_enum->numbered) { cdata++; csize--; + bsize--; }
rsize = hid_compute_report_size(report); @@ -2063,11 +2071,16 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 * else if (rsize > max_buffer_size) rsize = max_buffer_size;
+ if (bsize < rsize) { + hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %ld)\n", + report->id, rsize, bsize); + return -EINVAL; + } + if (csize < rsize) { - hid_warn_ratelimited(hid, "Event data for report %d was too short (%d vs %d)\n", - report->id, rsize, csize); - ret = -EINVAL; - goto out; + dbg_hid("report %d is too short, (%d < %d)\n", report->id, + csize, rsize); + memset(cdata + csize, 0, rsize - csize); }
if ((hid->claimed & HID_CLAIMED_HIDDEV) && hid->hiddev_report_event) @@ -2075,7 +2088,7 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 * if (hid->claimed & HID_CLAIMED_HIDRAW) { ret = hidraw_report_event(hid, data, size); if (ret) - goto out; + return ret; }
if (hid->claimed != HID_CLAIMED_HIDRAW && report->maxfield) { @@ -2087,15 +2100,15 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *
if (hid->claimed & HID_CLAIMED_INPUT) hidinput_report_event(hid, report); -out: + return ret; } EXPORT_SYMBOL_GPL(hid_report_raw_event);
static int __hid_input_report(struct hid_device *hid, enum hid_report_type type, - u8 *data, u32 size, int interrupt, u64 source, bool from_bpf, - bool lock_already_taken) + u8 *data, size_t bufsize, u32 size, int interrupt, u64 source, + bool from_bpf, bool lock_already_taken) { struct hid_report_enum *report_enum; struct hid_driver *hdrv; @@ -2120,7 +2133,8 @@ static int __hid_input_report(struct hid_device *hid, enum hid_report_type type, report_enum = hid->report_enum + type; hdrv = hid->driver;
- data = dispatch_hid_bpf_device_event(hid, type, data, &size, interrupt, source, from_bpf); + data = dispatch_hid_bpf_device_event(hid, type, data, &bufsize, &size, interrupt, + source, from_bpf); if (IS_ERR(data)) { ret = PTR_ERR(data); goto unlock; @@ -2149,7 +2163,7 @@ static int __hid_input_report(struct hid_device *hid, enum hid_report_type type, goto unlock; }
- ret = hid_report_raw_event(hid, type, data, size, interrupt); + ret = hid_report_raw_event(hid, type, data, bufsize, size, interrupt);
unlock: if (!lock_already_taken) @@ -2171,7 +2185,7 @@ static int __hid_input_report(struct hid_device *hid, enum hid_report_type type, int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, int interrupt) { - return __hid_input_report(hid, type, data, size, interrupt, 0, + return __hid_input_report(hid, type, data, size, size, interrupt, 0, false, /* from_bpf */ false /* lock_already_taken */); } diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c index 699186ff2349..d2a56bf92b41 100644 --- a/drivers/hid/hid-gfrm.c +++ b/drivers/hid/hid-gfrm.c @@ -66,7 +66,7 @@ static int gfrm_raw_event(struct hid_device *hdev, struct hid_report *report, switch (data[1]) { case GFRM100_SEARCH_KEY_DOWN: ret = hid_report_raw_event(hdev, HID_INPUT_REPORT, search_key_dn, - sizeof(search_key_dn), 1); + sizeof(search_key_dn), sizeof(search_key_dn), 1); break;
case GFRM100_SEARCH_KEY_AUDIO_DATA: @@ -74,7 +74,7 @@ static int gfrm_raw_event(struct hid_device *hdev, struct hid_report *report,
case GFRM100_SEARCH_KEY_UP: ret = hid_report_raw_event(hdev, HID_INPUT_REPORT, search_key_up, - sizeof(search_key_up), 1); + sizeof(search_key_up), sizeof(search_key_up), 1); break;
default: diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c index b1330d23bd2d..b3ff9265377b 100644 --- a/drivers/hid/hid-logitech-hidpp.c +++ b/drivers/hid/hid-logitech-hidpp.c @@ -3673,7 +3673,7 @@ static int hidpp10_consumer_keys_raw_event(struct hidpp_device *hidpp, memcpy(&consumer_report[1], &data[3], 4); /* We are called from atomic context */ hid_report_raw_event(hidpp->hid_dev, HID_INPUT_REPORT, - consumer_report, 5, 1); + consumer_report, sizeof(consumer_report), 5, 1);
return 1; } diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index e82a3c4e5b44..eeab0b6e32cc 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -533,7 +533,7 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) }
ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, - size, 0); + size, size, 0); if (ret) dev_warn(&hdev->dev, "failed to report feature\n"); } diff --git a/drivers/hid/hid-primax.c b/drivers/hid/hid-primax.c index e44d79dff8de..8db054280afb 100644 --- a/drivers/hid/hid-primax.c +++ b/drivers/hid/hid-primax.c @@ -44,7 +44,7 @@ static int px_raw_event(struct hid_device *hid, struct hid_report *report, data[0] |= (1 << (data[idx] - 0xE0)); data[idx] = 0; } - hid_report_raw_event(hid, HID_INPUT_REPORT, data, size, 0); + hid_report_raw_event(hid, HID_INPUT_REPORT, data, size, size, 0); return 1;
default: /* unknown report */ diff --git a/drivers/hid/hid-vivaldi-common.c b/drivers/hid/hid-vivaldi-common.c index bf734055d4b6..b12bb5cc091a 100644 --- a/drivers/hid/hid-vivaldi-common.c +++ b/drivers/hid/hid-vivaldi-common.c @@ -85,7 +85,7 @@ void vivaldi_feature_mapping(struct hid_device *hdev, }
ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, report_data, - report_len, 0); + report_len, report_len, 0); if (ret) { dev_warn(&hdev->dev, "failed to report feature %d\n", field->report->id); diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 0d1c6d90fe21..a32320b351e3 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -90,7 +90,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev, kfree(buf); continue; } - err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, false); + err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, size, false); if (err) { hid_warn(hdev, "%s: unable to flush event due to error %d\n", __func__, err); @@ -334,7 +334,7 @@ static void wacom_feature_mapping(struct hid_device *hdev, data, n, WAC_CMD_RETRIES); if (ret == n && features->type == HID_GENERIC) { ret = hid_report_raw_event(hdev, - HID_FEATURE_REPORT, data, n, 0); + HID_FEATURE_REPORT, data, n, n, 0); } else if (ret == 2 && features->type != HID_GENERIC) { features->touch_max = data[1]; } else { @@ -395,7 +395,7 @@ static void wacom_feature_mapping(struct hid_device *hdev, data, n, WAC_CMD_RETRIES); if (ret == n) { ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, - data, n, 0); + data, n, n, 0); } else { hid_warn(hdev, "%s: could not retrieve sensor offsets\n", __func__); diff --git a/drivers/staging/greybus/hid.c b/drivers/staging/greybus/hid.c index 1f58c907c036..f1f9f6fbc00e 100644 --- a/drivers/staging/greybus/hid.c +++ b/drivers/staging/greybus/hid.c @@ -201,7 +201,7 @@ static void gb_hid_init_report(struct gb_hid *ghid, struct hid_report *report) * we just need to setup the input fields, so using * hid_report_raw_event is safe. */ - hid_report_raw_event(ghid->hid, report->type, ghid->inbuf, size, 1); + hid_report_raw_event(ghid->hid, report->type, ghid->inbuf, ghid->bufsize, size, 1); }
static void gb_hid_init_reports(struct gb_hid *ghid) diff --git a/include/linux/hid.h b/include/linux/hid.h index 442a80d79e89..ac432a2ef415 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1298,8 +1298,8 @@ static inline u32 hid_report_len(struct hid_report *report) return DIV_ROUND_UP(report->size, 8) + (report->id > 0); }
-int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, - int interrupt); +int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 *data, + size_t bufsize, u32 size, int interrupt);
/* HID quirks API */ unsigned long hid_lookup_quirk(const struct hid_device *hdev); diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h index a2e47dbcf82c..19fffa4574a4 100644 --- a/include/linux/hid_bpf.h +++ b/include/linux/hid_bpf.h @@ -72,8 +72,8 @@ struct hid_ops { int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len, u64 source, bool from_bpf); int (*hid_input_report)(struct hid_device *hid, enum hid_report_type type, - u8 *data, u32 size, int interrupt, u64 source, bool from_bpf, - bool lock_already_taken); + u8 *data, size_t bufsize, u32 size, int interrupt, u64 source, + bool from_bpf, bool lock_already_taken); struct module *owner; const struct bus_type *bus_type; }; @@ -200,7 +200,8 @@ struct hid_bpf {
#ifdef CONFIG_HID_BPF u8 *dispatch_hid_bpf_device_event(struct hid_device *hid, enum hid_report_type type, u8 *data, - u32 *size, int interrupt, u64 source, bool from_bpf); + size_t *buf_size, u32 *size, int interrupt, u64 source, + bool from_bpf); int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, unsigned char reportnum, __u8 *buf, u32 size, enum hid_report_type rtype, @@ -215,8 +216,11 @@ int hid_bpf_device_init(struct hid_device *hid); const u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, const u8 *rdesc, unsigned int *size); #else /* CONFIG_HID_BPF */ static inline u8 *dispatch_hid_bpf_device_event(struct hid_device *hid, enum hid_report_type type, - u8 *data, u32 *size, int interrupt, - u64 source, bool from_bpf) { return data; } + u8 *data, size_t *buf_size, u32 *size, + int interrupt, u64 source, bool from_bpf) +{ + return data; +} static inline int dispatch_hid_bpf_raw_requests(struct hid_device *hdev, unsigned char reportnum, u8 *buf, u32 size, enum hid_report_type rtype,
On Mon, May 04, 2026 at 10:47:22AM +0200, Benjamin Tissoires wrote:
commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow. However, we can try to be smarter by providing both the buffer size and the data size, meaning that hid_report_raw_event() can make better decision whether we should plaining reject the buffer (buffer overflow attempt) or if we can safely memset it to 0 and pass it to the rest of the stack.
Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") Cc: stable@vger.kernel.org Signed-off-by: Benjamin Tissoires bentiss@kernel.org
drivers/hid/bpf/hid_bpf_dispatch.c | 6 ++++-- drivers/hid/hid-core.c | 42 +++++++++++++++++++++++++------------- drivers/hid/hid-gfrm.c | 4 ++-- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-multitouch.c | 2 +- drivers/hid/hid-primax.c | 2 +- drivers/hid/hid-vivaldi-common.c | 2 +- drivers/hid/wacom_sys.c | 6 +++--- drivers/staging/greybus/hid.c | 2 +-
The Greybus change builds fine now:
Acked-by: Johan Hovold johan@kernel.org
include/linux/hid.h | 4 ++-- include/linux/hid_bpf.h | 14 ++++++++----- 11 files changed, 53 insertions(+), 33 deletions(-)
Johan
On Mon, May 04, 2026 at 10:47:22AM +0200, Benjamin Tissoires wrote:
commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow. However, we can try to be smarter by providing both the buffer size and the data size, meaning that hid_report_raw_event() can make better decision whether we should plaining reject the buffer (buffer overflow attempt) or if we can safely memset it to 0 and pass it to the rest of the stack.
Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") Cc: stable@vger.kernel.org Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Reviewed-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
hid_input_report() is used in too many places to have a commit that doesn't cross subsystem borders. Instead of changing the API, introduce a new one when things matters in the transport layers: - usbhid - i2chid
This effectively revert to the old behavior for those two transport layers.
Fixes: 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") Cc: stable@vger.kernel.org Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- drivers/hid/hid-core.c | 25 +++++++++++++++++++++++++ drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++++--- drivers/hid/usbhid/hid-core.c | 11 ++++++----- include/linux/hid.h | 2 ++ 4 files changed, 37 insertions(+), 8 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index a806820df7e5..b3596851c719 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2181,6 +2181,7 @@ static int __hid_input_report(struct hid_device *hid, enum hid_report_type type, * @interrupt: distinguish between interrupt and control transfers * * This is data entry for lower layers. + * Legacy, please use hid_safe_input_report() instead. */ int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, int interrupt) @@ -2191,6 +2192,30 @@ int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data } EXPORT_SYMBOL_GPL(hid_input_report);
+/** + * hid_safe_input_report - report data from lower layer (usb, bt...) + * + * @hid: hid device + * @type: HID report type (HID_*_REPORT) + * @data: report contents + * @bufsize: allocated size of the data buffer + * @size: useful size of data parameter + * @interrupt: distinguish between interrupt and control transfers + * + * This is data entry for lower layers. + * Please use this function instead of the non safe version because we provide + * here the size of the buffer, allowing hid-core to make smarter decisions + * regarding the incoming buffer. + */ +int hid_safe_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, + size_t bufsize, u32 size, int interrupt) +{ + return __hid_input_report(hid, type, data, bufsize, size, interrupt, 0, + false, /* from_bpf */ + false /* lock_already_taken */); +} +EXPORT_SYMBOL_GPL(hid_safe_input_report); + bool hid_match_one_id(const struct hid_device *hdev, const struct hid_device_id *id) { diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c index 5a183af3d5c6..e0a302544cef 100644 --- a/drivers/hid/i2c-hid/i2c-hid-core.c +++ b/drivers/hid/i2c-hid/i2c-hid-core.c @@ -574,9 +574,10 @@ static void i2c_hid_get_input(struct i2c_hid *ihid) if (ihid->hid->group != HID_GROUP_RMI) pm_wakeup_event(&ihid->client->dev, 0);
- hid_input_report(ihid->hid, HID_INPUT_REPORT, - ihid->inbuf + sizeof(__le16), - ret_size - sizeof(__le16), 1); + hid_safe_input_report(ihid->hid, HID_INPUT_REPORT, + ihid->inbuf + sizeof(__le16), + ihid->bufsize - sizeof(__le16), + ret_size - sizeof(__le16), 1); }
return; diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c index fbbfc0f60829..5af93b9b1fb5 100644 --- a/drivers/hid/usbhid/hid-core.c +++ b/drivers/hid/usbhid/hid-core.c @@ -283,9 +283,9 @@ static void hid_irq_in(struct urb *urb) break; usbhid_mark_busy(usbhid); if (!test_bit(HID_RESUME_RUNNING, &usbhid->iofl)) { - hid_input_report(urb->context, HID_INPUT_REPORT, - urb->transfer_buffer, - urb->actual_length, 1); + hid_safe_input_report(urb->context, HID_INPUT_REPORT, + urb->transfer_buffer, urb->transfer_buffer_length, + urb->actual_length, 1); /* * autosuspend refused while keys are pressed * because most keyboards don't wake up when @@ -482,9 +482,10 @@ static void hid_ctrl(struct urb *urb) switch (status) { case 0: /* success */ if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_IN) - hid_input_report(urb->context, + hid_safe_input_report(urb->context, usbhid->ctrl[usbhid->ctrltail].report->type, - urb->transfer_buffer, urb->actual_length, 0); + urb->transfer_buffer, urb->transfer_buffer_length, + urb->actual_length, 0); break; case -ESHUTDOWN: /* unplug */ unplug = 1; diff --git a/include/linux/hid.h b/include/linux/hid.h index ac432a2ef415..bfb9859f391e 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -1030,6 +1030,8 @@ struct hid_field *hid_find_field(struct hid_device *hdev, unsigned int report_ty int hid_set_field(struct hid_field *, unsigned, __s32); int hid_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, u32 size, int interrupt); +int hid_safe_input_report(struct hid_device *hid, enum hid_report_type type, u8 *data, + size_t bufsize, u32 size, int interrupt); struct hid_field *hidinput_get_led_field(struct hid_device *hid); unsigned int hidinput_count_leds(struct hid_device *hid); __s32 hidinput_calc_abs_res(const struct hid_field *field, __u16 code);
This simplifies error handling and protects against memory leaks.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- drivers/hid/hid-multitouch.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c index eeab0b6e32cc..b19463e545d6 100644 --- a/drivers/hid/hid-multitouch.c +++ b/drivers/hid/hid-multitouch.c @@ -507,7 +507,6 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) { int ret; u32 size = hid_report_len(report); - u8 *buf;
/* * Do not fetch the feature report if the device has been explicitly @@ -516,7 +515,7 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) if (hdev->quirks & HID_QUIRK_NO_INIT_REPORTS) return;
- buf = hid_alloc_report_buf(report, GFP_KERNEL); + u8 *buf __free(kfree) = hid_alloc_report_buf(report, GFP_KERNEL); if (!buf) return;
@@ -529,7 +528,7 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) /* The report ID in the request and the response should match */ if (report->id != buf[0]) { hid_err(hdev, "Returned feature report did not match the request\n"); - goto free; + return; }
ret = hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, @@ -537,9 +536,6 @@ static void mt_get_feature(struct hid_device *hdev, struct hid_report *report) if (ret) dev_warn(&hdev->dev, "failed to report feature\n"); } - -free: - kfree(buf); }
static void mt_feature_mapping(struct hid_device *hdev, @@ -1658,7 +1654,6 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev, struct mt_class *cls = &td->mtclass; struct hid_report *report = field->report; unsigned int index = usage->usage_index; - char *buf; u32 report_len; int max;
@@ -1673,17 +1668,18 @@ static bool mt_need_to_apply_feature(struct hid_device *hdev, return false;
if (cls->quirks & MT_QUIRK_FORCE_GET_FEATURE) { - report_len = hid_report_len(report); - buf = hid_alloc_report_buf(report, GFP_KERNEL); + char *buf __free(kfree) = hid_alloc_report_buf(report, GFP_KERNEL); + if (!buf) { hid_err(hdev, "failed to allocate buffer for report\n"); return false; } + + report_len = hid_report_len(report); hid_hw_raw_request(hdev, report->id, buf, report_len, HID_FEATURE_REPORT, HID_REQ_GET_REPORT); - kfree(buf); }
field->value[index] = td->inputmode_value;
This simplifies error handling and protects against memory leaks.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- drivers/hid/hid-core.c | 4 ++-- drivers/hid/wacom_sys.c | 40 +++++++++++++--------------------------- 2 files changed, 15 insertions(+), 29 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index b3596851c719..7fc20e0405ad 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -2050,8 +2050,8 @@ int hid_report_raw_event(struct hid_device *hid, enum hid_report_type type, u8 * return 0;
if (unlikely(bsize < csize)) { - hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %ld)\n", - report->id, csize, bsize); + hid_warn_ratelimited(hid, "Event data for report %d is incorrect (%d vs %d)\n", + report->id, csize, (unsigned int)bsize); return -EINVAL; }
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index a32320b351e3..adb31f54e524 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c @@ -70,11 +70,10 @@ static void wacom_wac_queue_flush(struct hid_device *hdev, { while (!kfifo_is_empty(fifo)) { int size = kfifo_peek_len(fifo); - u8 *buf; unsigned int count; int err;
- buf = kzalloc(size, GFP_KERNEL); + u8 *buf __free(kfree) = kzalloc(size, GFP_KERNEL); if (!buf) { kfifo_skip(fifo); continue; @@ -87,7 +86,6 @@ static void wacom_wac_queue_flush(struct hid_device *hdev, // to flush seems reasonable enough, however. hid_warn(hdev, "%s: removed fifo entry with unexpected size\n", __func__); - kfree(buf); continue; } err = hid_report_raw_event(hdev, HID_INPUT_REPORT, buf, size, size, false); @@ -95,8 +93,6 @@ static void wacom_wac_queue_flush(struct hid_device *hdev, hid_warn(hdev, "%s: unable to flush event due to error %d\n", __func__, err); } - - kfree(buf); } }
@@ -311,7 +307,6 @@ static void wacom_feature_mapping(struct hid_device *hdev, struct wacom_features *features = &wacom->wacom_wac.features; struct hid_data *hid_data = &wacom->wacom_wac.hid_data; unsigned int equivalent_usage = wacom_equivalent_usage(usage->hid); - u8 *data; int ret; u32 n;
@@ -325,10 +320,11 @@ static void wacom_feature_mapping(struct hid_device *hdev, /* leave touch_max as is if predefined */ if (!features->touch_max) { /* read manually */ - n = hid_report_len(field->report); - data = hid_alloc_report_buf(field->report, GFP_KERNEL); + u8 *data __free(kfree) = hid_alloc_report_buf(field->report, GFP_KERNEL); + if (!data) break; + n = hid_report_len(field->report); data[0] = field->report->id; ret = wacom_get_report(hdev, HID_FEATURE_REPORT, data, n, WAC_CMD_RETRIES); @@ -344,7 +340,6 @@ static void wacom_feature_mapping(struct hid_device *hdev, "defaulting to %d\n", features->touch_max); } - kfree(data); } break; case HID_DG_INPUTMODE: @@ -386,10 +381,11 @@ static void wacom_feature_mapping(struct hid_device *hdev, case WACOM_HID_WD_OFFSETRIGHT: case WACOM_HID_WD_OFFSETBOTTOM: /* read manually */ - n = hid_report_len(field->report); - data = hid_alloc_report_buf(field->report, GFP_KERNEL); + u8 *data __free(kfree) = hid_alloc_report_buf(field->report, GFP_KERNEL); + if (!data) break; + n = hid_report_len(field->report); data[0] = field->report->id; ret = wacom_get_report(hdev, HID_FEATURE_REPORT, data, n, WAC_CMD_RETRIES); @@ -400,7 +396,6 @@ static void wacom_feature_mapping(struct hid_device *hdev, hid_warn(hdev, "%s: could not retrieve sensor offsets\n", __func__); } - kfree(data); break; } } @@ -581,7 +576,6 @@ static int wacom_hid_set_device_mode(struct hid_device *hdev) static int wacom_set_device_mode(struct hid_device *hdev, struct wacom_wac *wacom_wac) { - u8 *rep_data; struct hid_report *r; struct hid_report_enum *re; u32 length; @@ -595,7 +589,7 @@ static int wacom_set_device_mode(struct hid_device *hdev, if (!r) return -EINVAL;
- rep_data = hid_alloc_report_buf(r, GFP_KERNEL); + u8 *rep_data __free(kfree) = hid_alloc_report_buf(r, GFP_KERNEL); if (!rep_data) return -ENOMEM;
@@ -614,8 +608,6 @@ static int wacom_set_device_mode(struct hid_device *hdev, rep_data[1] != wacom_wac->mode_report && limit++ < WAC_MSG_RETRIES);
- kfree(rep_data); - return error < 0 ? error : 0; }
@@ -921,7 +913,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
static int wacom_led_control(struct wacom *wacom) { - unsigned char *buf; int retval; unsigned char report_id = WAC_CMD_LED_CONTROL; int buf_size = 9; @@ -940,7 +931,8 @@ static int wacom_led_control(struct wacom *wacom) report_id = WAC_CMD_WL_INTUOSP2; buf_size = 51; } - buf = kzalloc(buf_size, GFP_KERNEL); + + unsigned char *buf __free(kfree) = kzalloc(buf_size, GFP_KERNEL); if (!buf) return -ENOMEM;
@@ -996,7 +988,6 @@ static int wacom_led_control(struct wacom *wacom)
retval = wacom_set_report(wacom->hdev, HID_FEATURE_REPORT, buf, buf_size, WAC_CMD_RETRIES); - kfree(buf);
return retval; } @@ -1004,11 +995,10 @@ static int wacom_led_control(struct wacom *wacom) static int wacom_led_putimage(struct wacom *wacom, int button_id, u8 xfer_id, const unsigned len, const void *img) { - unsigned char *buf; int i, retval; const unsigned chunk_len = len / 4; /* 4 chunks are needed to be sent */
- buf = kzalloc(chunk_len + 3 , GFP_KERNEL); + unsigned char *buf __free(kfree) = kzalloc(chunk_len + 3, GFP_KERNEL); if (!buf) return -ENOMEM;
@@ -1018,7 +1008,7 @@ static int wacom_led_putimage(struct wacom *wacom, int button_id, u8 xfer_id, retval = wacom_set_report(wacom->hdev, HID_FEATURE_REPORT, buf, 2, WAC_CMD_RETRIES); if (retval < 0) - goto out; + return retval;
buf[0] = xfer_id; buf[1] = button_id & 0x07; @@ -1038,8 +1028,6 @@ static int wacom_led_putimage(struct wacom *wacom, int button_id, u8 xfer_id, wacom_set_report(wacom->hdev, HID_FEATURE_REPORT, buf, 2, WAC_CMD_RETRIES);
-out: - kfree(buf); return retval; }
@@ -1948,10 +1936,9 @@ static int wacom_remote_create_attr_group(struct wacom *wacom, __u32 serial, static int wacom_cmd_unpair_remote(struct wacom *wacom, unsigned char selector) { const size_t buf_size = 2; - unsigned char *buf; int retval;
- buf = kzalloc(buf_size, GFP_KERNEL); + unsigned char *buf __free(kfree) = kzalloc(buf_size, GFP_KERNEL); if (!buf) return -ENOMEM;
@@ -1960,7 +1947,6 @@ static int wacom_cmd_unpair_remote(struct wacom *wacom, unsigned char selector)
retval = wacom_set_report(wacom->hdev, HID_OUTPUT_REPORT, buf, buf_size, WAC_CMD_RETRIES); - kfree(buf);
return retval; }
Hi Benjamin,
On Mon, May 04, 2026 at 10:47:25AM +0200, Benjamin Tissoires wrote:
@@ -386,10 +381,11 @@ static void wacom_feature_mapping(struct hid_device *hdev, case WACOM_HID_WD_OFFSETRIGHT: case WACOM_HID_WD_OFFSETBOTTOM: /* read manually */
n = hid_report_len(field->report);data = hid_alloc_report_buf(field->report, GFP_KERNEL);
u8 *data __free(kfree) = hid_alloc_report_buf(field->report, GFP_KERNEL);- if (!data) break;
n = hid_report_len(field->report);@@ -400,7 +396,6 @@ static void wacom_feature_mapping(struct hid_device *hdev, hid_warn(hdev, "%s: could not retrieve sensor offsets\n", __func__); }
kfree(data);
I'd recommend establishing a new scope for the "data", otherwise it is fragile. If there was another label below then this cleanup would explode since current scope of "data" is from the declaration point until the end of the switch statement.
Having a dedicated scope makes lifertime explicit.
Thanks.
On Mon, 04 May 2026, Benjamin Tissoires wrote:
Commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow.
We only had corner cases of malicious devices exposing the OOM because in most cases, the buffer provided by the transport layer needs to be allocated at probe time and is large enough to handle all the possible reports.
However, the patch from above, which enforces the spec a little bit more introduced both regressions for devices not following the spec (not necesserally malicious), but also a stream of errors for those devices.
Let's revert to the old behavior by giving more information to HID core to be able to decide whether it can or not memset the rest of the buffer to 0 and continue the processing.
Note that the first commit makes an API change, but the callers are relatively limited, so it should be fine on its own. The second patch can't really make the same kind of API change because we have too many callers in various subsystems. We can switch them one by one to the safe approach when needed.
The last 2 patches are small cleanups I initially put together with the 2 first patches, but they can be applied on their own and don't need to be pulled in stable like the first 2.
Cheers, Benjamin
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Changes in v3:
- fixed ghib -> ghid in greybus
- fixed i386 size_t debug size reported by kernel-bot
- Link to v2: https://lore.kernel.org/r/20260416-wip-fix-core-v2-0-be92570e5627@kernel.org
Changes in v2:
- added a small blurb explaining the difference between the safe and the non safe version of hid_safe_input_report
- Link to v1: https://lore.kernel.org/r/20260415-wip-fix-core-v1-0-ed3c4c823175@kernel.org
Benjamin Tissoires (4): HID: pass the buffer size to hid_report_raw_event HID: core: introduce hid_safe_input_report() HID: multitouch: use __free(kfree) to clean up temporary buffers HID: wacom: use __free(kfree) to clean up temporary buffers
drivers/hid/bpf/hid_bpf_dispatch.c | 6 ++-- drivers/hid/hid-core.c | 67 ++++++++++++++++++++++++++++++-------- drivers/hid/hid-gfrm.c | 4 +-- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-multitouch.c | 18 ++++------ drivers/hid/hid-primax.c | 2 +- drivers/hid/hid-vivaldi-common.c | 2 +- drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++-- drivers/hid/usbhid/hid-core.c | 11 ++++--- drivers/hid/wacom_sys.c | 46 +++++++++----------------- drivers/staging/greybus/hid.c | 2 +- include/linux/hid.h | 6 ++-- include/linux/hid_bpf.h | 14 +++++--- 13 files changed, 109 insertions(+), 78 deletions(-)
What's the plan for this set Benjamin? -rcs or -next?
On Wed, 06 May 2026, Lee Jones wrote:
On Mon, 04 May 2026, Benjamin Tissoires wrote:
Commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow.
We only had corner cases of malicious devices exposing the OOM because in most cases, the buffer provided by the transport layer needs to be allocated at probe time and is large enough to handle all the possible reports.
However, the patch from above, which enforces the spec a little bit more introduced both regressions for devices not following the spec (not necesserally malicious), but also a stream of errors for those devices.
Let's revert to the old behavior by giving more information to HID core to be able to decide whether it can or not memset the rest of the buffer to 0 and continue the processing.
Note that the first commit makes an API change, but the callers are relatively limited, so it should be fine on its own. The second patch can't really make the same kind of API change because we have too many callers in various subsystems. We can switch them one by one to the safe approach when needed.
The last 2 patches are small cleanups I initially put together with the 2 first patches, but they can be applied on their own and don't need to be pulled in stable like the first 2.
Cheers, Benjamin
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Changes in v3:
- fixed ghib -> ghid in greybus
- fixed i386 size_t debug size reported by kernel-bot
- Link to v2: https://lore.kernel.org/r/20260416-wip-fix-core-v2-0-be92570e5627@kernel.org
Changes in v2:
- added a small blurb explaining the difference between the safe and the non safe version of hid_safe_input_report
- Link to v1: https://lore.kernel.org/r/20260415-wip-fix-core-v1-0-ed3c4c823175@kernel.org
Benjamin Tissoires (4): HID: pass the buffer size to hid_report_raw_event HID: core: introduce hid_safe_input_report() HID: multitouch: use __free(kfree) to clean up temporary buffers HID: wacom: use __free(kfree) to clean up temporary buffers
drivers/hid/bpf/hid_bpf_dispatch.c | 6 ++-- drivers/hid/hid-core.c | 67 ++++++++++++++++++++++++++++++-------- drivers/hid/hid-gfrm.c | 4 +-- drivers/hid/hid-logitech-hidpp.c | 2 +- drivers/hid/hid-multitouch.c | 18 ++++------ drivers/hid/hid-primax.c | 2 +- drivers/hid/hid-vivaldi-common.c | 2 +- drivers/hid/i2c-hid/i2c-hid-core.c | 7 ++-- drivers/hid/usbhid/hid-core.c | 11 ++++--- drivers/hid/wacom_sys.c | 46 +++++++++----------------- drivers/staging/greybus/hid.c | 2 +- include/linux/hid.h | 6 ++-- include/linux/hid_bpf.h | 14 +++++--- 13 files changed, 109 insertions(+), 78 deletions(-)
What's the plan for this set Benjamin? -rcs or -next?
Are there any updates on this set please?
FYI, this set is still important to us.
Ideally, if all is well, it would go into the -rcs for v7.1.
On Mon, 4 May 2026, Benjamin Tissoires wrote:
Commit 0a3fe972a7cb ("HID: core: Mitigate potential OOB by removing bogus memset()") enforced the provided data to be at least the size of the declared buffer in the report descriptor to prevent a buffer overflow.
We only had corner cases of malicious devices exposing the OOM because in most cases, the buffer provided by the transport layer needs to be allocated at probe time and is large enough to handle all the possible reports.
However, the patch from above, which enforces the spec a little bit more introduced both regressions for devices not following the spec (not necesserally malicious), but also a stream of errors for those devices.
Let's revert to the old behavior by giving more information to HID core to be able to decide whether it can or not memset the rest of the buffer to 0 and continue the processing.
Note that the first commit makes an API change, but the callers are relatively limited, so it should be fine on its own. The second patch can't really make the same kind of API change because we have too many callers in various subsystems. We can switch them one by one to the safe approach when needed.
The last 2 patches are small cleanups I initially put together with the 2 first patches, but they can be applied on their own and don't need to be pulled in stable like the first 2.
Cheers, Benjamin
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
I have now queued the first two in hid.git#for-7.1/upstream-fixes.
I expect the remaining two will be applied once respun with Dmitry's suggestion on proper guarding.
Thanks,
-
Benjamin Tissoires -
Dmitry Torokhov -
Greg Kroah-Hartman -
Jiri Kosina -
Johan Hovold -
Lee Jones