On Mon, Oct 19, 2026 at 12:26 PM <[email protected]> wrote: > > 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)?
Will fix this in v7. > > [ ... ] > > -- > Sashiko AI review · > https://sashiko.dev/#/patchset/[email protected]?part=4
