Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] HID: spi-hid: zero-length report descriptor accepted in spi_hid_ll_parse() --
commit 9328ec8c48a0b55cf1019d845ec2c37e89f4cdbb Author: Jingyuan Liang <[email protected]> HID: spi-hid: add spi-hid driver HID layer This commit adds the HID low-level driver callbacks and device creation logic for spi-hid, handling report descriptor fetching, buffer allocation, and output/feature report requests. > diff --git a/drivers/hid/spi-hid/spi-hid-core.c > b/drivers/hid/spi-hid/spi-hid-core.c > index 02a7608c4b882..fca7a44eeb9f9 100644 > --- a/drivers/hid/spi-hid/spi-hid-core.c > +++ b/drivers/hid/spi-hid/spi-hid-core.c [ ... ] > @@ -84,11 +185,484 @@ static const char *spi_hid_power_mode_string(enum > hidspi_power_state power_state [ ... ] > +/* > + * This function returns the length of the report descriptor, or a negative > + * error code if something went wrong. > + */ > +static int spi_hid_report_descriptor_request(struct spi_hid *shid) > +{ > + struct device *dev = &shid->spi->dev; > + struct spi_hid_output_report report = { > + .report_type = REPORT_DESCRIPTOR, > + .content_length = 0, > + .content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST, > + .content = NULL, > + }; > + int ret; > + > + ret = spi_hid_sync_request(shid, &report); > + if (ret) { > + dev_err(dev, > + "Expected report descriptor not received: %d\n", ret); > + return ret; > + } > + > + ret = shid->response_length; > + if (ret != shid->desc.report_descriptor_length) { > + ret = min_t(unsigned int, ret, > shid->desc.report_descriptor_length); > + dev_err(dev, "Received report descriptor length doesn't match > device descriptor field, using min of the two: %d\n", > + ret); > + } > + > + return ret; > +} [ ... ] > +static int spi_hid_ll_parse(struct hid_device *hid) > +{ > + struct spi_device *spi = hid->driver_data; > + struct spi_hid *shid = spi_get_drvdata(spi); > + struct device *dev = &spi->dev; > + unsigned int rsize = shid->desc.report_descriptor_length; > + int error, len; > + > + if (rsize > HID_MAX_DESCRIPTOR_SIZE) { > + dev_err(dev, > + "Report descriptor size %d is greater than > HID_MAX_DESCRIPTOR_SIZE %d\n", > + rsize, HID_MAX_DESCRIPTOR_SIZE); > + return -EINVAL; > + } > + > + if (rsize > shid->bufsize) { > + error = spi_hid_alloc_buffers(shid, rsize); > + if (error) > + return error; > + } > + > + len = spi_hid_report_descriptor_request(shid); > + if (len < 0) { > + dev_err(dev, "Report descriptor request failed, %d\n", len); > + return len; > + } > + > + /* > + * FIXME: below call returning 0 doesn't mean that the report descriptor > + * is good. We might be caching a crc32 of a corrupted r. d. or who > + * knows what the FW sent. Need to have a feedback loop about r. d. > + * being ok and only then cache it. > + */ > + error = hid_parse_report(hid, (u8 *)shid->response->content, len); > + if (error) { > + dev_err(dev, "failed parsing report: %d\n", error); > + return error; > + } > + shid->report_descriptor_crc32 = crc32_le(0, > + (unsigned char const > *)shid->response->content, > + len); > + > + set_bit(SPI_HID_READY, &shid->flags); > + > + return 0; > +} [Severity: Medium] What happens in spi_hid_ll_parse() if the report descriptor length is 0? In spi_hid_ll_parse(), rsize (shid->desc.report_descriptor_length) is checked against HID_MAX_DESCRIPTOR_SIZE, but not for zero. Later in the series, the commit "HID: spi-hid: add HID SPI protocol implementation" validates raw->rep_desc_len != 0 in spi_hid_dev_desc_response(), but if a device returns a REPORT_DESCRIPTOR_RESPONSE with content_length == 0 (shid->response_length == 0), spi_hid_report_descriptor_request() computes min_t(unsigned int, 0, shid->desc.report_descriptor_length) and returns 0. Both spi_hid_ll_parse() here and spi_hid_refresh_device() in the protocol patch only check if (len < 0), treating len == 0 as success. When len == 0 is passed to hid_parse_report(): drivers/hid/hid-core.c:hid_parse_report() { hid->dev_rdesc = kmemdup(start, size, GFP_KERNEL); if (!hid->dev_rdesc) return -ENOMEM; hid->dev_rsize = size; return 0; } kmemdup() returns ZERO_SIZE_PTR (non-NULL), so hid_parse_report() returns 0. spi_hid_ll_parse() then caches a CRC32 of 0 and sets SPI_HID_READY, and hid_add_device() sees a non-NULL hdev->dev_rdesc and calls device_add(), registering an unusable hid_device while bypassing SPI_HID_ERROR reset recovery. Should spi_hid_report_descriptor_request() or its callers reject a 0-length report descriptor (for example, checking !rsize in spi_hid_ll_parse() and returning -EINVAL when shid->response_length == 0 or len <= 0)? [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
