Re: [PATCH v6 2/3] HID: Add Himax HX83102J SPI touchscreen driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] hid: himax_hid_raw_request() returns requested length instead of actual bytes written, leaking uninitialized memory
- [High] hid: hardware swallows commands after himax_pin_reset() due to missing dummy bus read
- [Low] hid: himax_hid_add() fails to initialize mandatory hid_device fields (phys, uniq, country)
--

commit 09f1c425ef5c3359e150cf5f66b3a172b5847898
Author: Michał Kopeć <michal@nozomi.space>

HID: Add Himax HX83102J SPI touchscreen driver

This commit adds a new driver for the Himax HX83102J touch and display driver
integrated (TDDI) controller, connecting over SPI and sending HID reports.
The driver supports loading firmware for flashless variants and coordinates
power management with the display panel.

Link: https://lore.kernel.org/all/TY0PR06MB56110754EB9D827BE3304A259E0F2@TY0PR06MB5611.apcprd06.prod.outlook.com/ [1]
Link: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5326106 [2]

> diff --git a/drivers/hid/hid-himax-spi.c b/drivers/hid/hid-himax-spi.c
> new file mode 100644
> index 0000000000000..98ee51658b362
> --- /dev/null
> +++ b/drivers/hid/hid-himax-spi.c

[ ... ]

> +static int himax_sense_off(struct himax_ts_data *ts)
> +{
> +	unsigned int tries = 5;
> +	u32 val;
> +	int ret;
> +
> +	while (tries--) {
> +		ret = himax_enter_safe_mode(ts);
> +		if (ret)
> +			return ret;
> +
> +		ret = himax_read32(ts, HIMAX_REG_FW_STATUS, &val);
> +		if (ret)
> +			return ret;
> +
> +		if (FIELD_GET(HIMAX_FW_STATE, val) == HIMAX_FW_STATE_SAFE_MODE) {
> +			/* The touch MCU can't be used before the TCON is reset */
> +			ret = himax_write32(ts, HX83102J_REG_TCON_RST, 0);
> +			if (ret)
> +				return ret;
> +
> +			fsleep(1000);
> +			return 0;
> +		}
> +
> +		fsleep(5000);
> +		himax_pin_reset(ts);
> +	}
> +
> +	return -EIO;
> +}

[Severity: High]
Does this error recovery loop inadvertently swallow the safe mode command?

The documentation in himax_interface_on() states that the hardware swallows
the first SPI transaction after a pin reset. Here, himax_pin_reset(ts)
is called and the loop immediately restarts with himax_enter_safe_mode(ts).
Without a dummy read to wake the bus, the safe mode command itself might be
swallowed, which could prevent the IC from ever entering safe mode and
create an infinite failure loop.

> +static int himax_sense_on(struct himax_ts_data *ts)
> +{
> +	int ret;
> +
> +	ret = himax_interface_on(ts);
> +	if (ret)
> +		return ret;
> +
> +	ret = himax_write32(ts, HIMAX_REG_CTRL_FW, HIMAX_FW_RE_INIT);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(10000);
> +	himax_pin_reset(ts);
> +
> +	ret = himax_enable_hw_crc(ts);

[Severity: High]
Could the hardware swallow the first AHB configuration write here?

Similarly to the loop in himax_sense_off(), a hardware reset from
himax_pin_reset(ts) is immediately followed by a write function
(himax_enable_hw_crc). This means the first AHB configuration write
(HIMAX_BUS_AHB_CONTI inside himax_ahb_set_burst) might be swallowed
by the sleeping bus, potentially corrupting the AHB transaction state.

[ ... ]

> +static int himax_hid_raw_request(struct hid_device *hid, unsigned char reportnum,
> +				 u8 *buf, size_t len, unsigned char rtype, int reqtype)
> +{
> +	struct himax_ts_data *ts = hid->driver_data;
> +
> +	/* The only feature report the driver serves is the maximum contact count */
> +	if (reqtype != HID_REQ_GET_REPORT || reportnum != HIMAX_REPORT_ID_CONTACT_MAX ||
> +	    len < 2)
> +		return -EINVAL;
> +
> +	buf[0] = reportnum;
> +	buf[1] = ts->max_points;
> +
> +	return len;

[Severity: High]
Does this code leak uninitialized kernel heap memory to userspace?

The driver handles HID_REQ_GET_REPORT requests by populating exactly 2 bytes.
However, it unconditionally returns len (the size of the buffer requested
by the caller) rather than 2. 

When userspace issues an ioctl (like HIDIOCGFEATURE) with a length larger
than 2, the hidraw core allocates a buffer of size len using kmalloc(), which
does not zero memory. Because the driver returns len, hidraw copies the full
buffer back to userspace, leaking the remaining uninitialized bytes.

[ ... ]

> +static int himax_hid_add(struct himax_ts_data *ts)
> +{
> +	const struct himax_hid_desc *desc = ts->hid_desc;
> +	struct hid_device *hid;
> +	int ret;
> +
> +	if (!ts->event_buf) {
> +		ret = himax_read_ic_info(ts);
> +		if (ret)
> +			return ret;
> +
> +		ret = himax_alloc_report_bufs(ts);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	hid = hid_allocate_device();
> +	if (IS_ERR(hid))
> +		return PTR_ERR(hid);
> +
> +	hid->driver_data = ts;
> +	hid->ll_driver = &himax_hid_ll_driver;
> +	hid->bus = BUS_SPI;
> +	hid->dev.parent = ts->dev;
> +	hid->version = le16_to_cpu(desc->bcd_version);
> +	hid->vendor = le16_to_cpu(desc->vendor_id);
> +	hid->product = le16_to_cpu(desc->product_id);
> +	snprintf(hid->name, sizeof(hid->name), "hid-hxtp %04X:%04X",
> +		 hid->vendor, hid->product);
> +
> +	ret = hid_add_device(hid);

[Severity: Low]
Are there missing initializations for mandatory hid_device fields here?

According to the HID Subsystem Guide, before registration, the transport
driver must fully initialize the hid_device fields, including phys, uniq,
and country. While hid_allocate_device() zeroes the structure, explicitly
setting these fields is a documented subsystem requirement for all
transport drivers before calling hid_add_device().

[ ... ]

> +static irqreturn_t himax_irq(int irq, void *data)
> +{
> +	struct himax_ts_data *ts = data;
> +	size_t len = ts->max_input_len - HIMAX_REPORT_HDR_SIZE;
> +	size_t pos = HIMAX_REPORT_HDR_SIZE;
> +	int ret;
> +
> +	ret = himax_read_events(ts);
> +	if (ret) {
> +		dev_warn_ratelimited(ts->dev, "failed to read events: %d\n", ret);
> +		himax_pin_reset(ts);
> +		himax_reload_to_active(ts);
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
Is a dummy bus read also needed here during error recovery?

The error recovery path calls himax_pin_reset(ts) and directly writes to
HIMAX_REG_RELOAD_TO_ACTIVE. Just like the reset cases in himax_sense_off()
and himax_sense_on(), this command might be swallowed by the sleeping bus,
leaving the hardware in an inconsistent state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011181633.1778969-1-michal@nozomi.space?part=2