Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] The patch introduces a hard build failure by passing 9 arguments to a function expecting 8.
Pre-existing issues:
- [High] Integer underflow in `i2c_hid_get_input()` when computing the remaining buffer size for bogus HID descriptors.
--
commit d54718e72e548c203a2c42b46b69bf352c081a00
Author: Benjamin Tissoires [off-list ref]
HID: core: introduce hid_safe_input_report()
This patch introduces hid_safe_input_report() to be used in transport
layers like usbhid and i2chid. This provides the buffer size allowing
the hid-core to make smarter decisions regarding incoming buffers.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 294a25330ed03..2c5583818421e 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -2141,6 +2142,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 */);
+}
[Severity: Low]
Does this cause a build failure? Looking at the definition of
__hid_input_report() in this file, it only accepts 8 arguments:
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)
This call passes 9 arguments, including the new bufsize parameter. Were some
prerequisite upstream patches updating the signature of __hid_input_report()
omitted from this backport?
quoted hunk ↗ jump to hunk
+EXPORT_SYMBOL_GPL(hid_safe_input_report);
+
bool hid_match_one_id(const struct hid_device *hdev,
const struct hid_device_id *id)
[ ... ]
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index cf8ae0df0cda9..8ce0535fc42d6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -568,9 +568,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);
[Severity: High]
This is a pre-existing issue, but can this calculation underflow and pass a
massive size to the HID core?
If a device has the I2C_HID_QUIRK_BAD_INPUT_SIZE quirk and provides a bogus
descriptor with wMaxInputLength < 2, the size variable will be < 2, and the
code earlier in i2c_hid_get_input() forces ret_size = size.
When ret_size and ihid->bufsize are less than sizeof(__le16) (which is 2),
subtracting sizeof(__le16) will result in an unsigned integer underflow.
This large underflowed value would then be passed into the HID core here and
could lead to out-of-bounds reads in functions like hid_dump_report().
}
return;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260601083642.908433-3-lee@kernel.org?part=1