Re: [PATCH 3/5] HID: intel-thc-hid: intel-ipts: expose the port as a HID device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] HID: intel-thc-hid: zero return on SET_REPORT in
  thc_ipts_hid_raw_request()
- [High] HID: intel-thc-hid: DMA stopped before HID removal in
  thc_ipts_port_stop()
- [High] HID: intel-thc-hid: uninitialized frame len in
  thc_ipts_read_frames()
- [Medium] HID: intel-thc-hid: unvalidated feature report ID in
  thc_ipts_get_feature()
- [Medium] HID: intel-thc-hid: racy and zero-length descriptor in
  thc_ipts_descriptor()
--

commit 378fb3be497c5d8bed8daca37c866a233cfb6aad
Author: Emfox Zhou <emfox@debian.org>

HID: intel-thc-hid: intel-ipts: expose the port as a HID device

This commit registers a HID device for the IPTS port, adds TX frame helpers,
and implements report descriptor parsing, feature report handshakes, and
input report forwarding.

> diff --git a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> index 8529b91e063f7..55206205f2115 100644
> --- a/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
> +++ b/drivers/hid/intel-thc-hid/intel-ipts/ipts.c
[ ... ]
> @@ -135,8 +137,62 @@ static int thc_ipts_touch_set(struct thc_ipts *idev, bool enable)
>  				 &touch);
>  }
>  
> +/*
> + * One frame to the controller: code, length, then the payload padded to four
> + * bytes. The frame is built in @idev->tx_frame, so callers serialise with
> + * @idev->req_lock.
> + */
> +static int thc_ipts_send(struct thc_ipts *idev, u32 code, const void *data,
> +			 size_t len)
> +{
> +	size_t padded = round_up(len, 4);
> +	u8 *frame = idev->tx_frame;
> +
> +	if (!idev->dma_ready)
> +		return -EAGAIN;
> +	if (THC_IPTS_TX_HDR_SIZE + padded > idev->max_tx)
> +		return -EMSGSIZE;
> +
> +	put_unaligned_le32(code, frame);
> +	put_unaligned_le32((u32)len, frame + 4);
> +	memset(frame + THC_IPTS_TX_HDR_SIZE, 0, padded);
> +	if (len && data)
> +		memcpy(frame + THC_IPTS_TX_HDR_SIZE, data, len);
> +
> +	return thc_dma_write(idev->thc, frame, THC_IPTS_TX_HDR_SIZE + padded);
> +}
> +
> +static void thc_ipts_descriptor(struct thc_ipts *idev, const u8 *data,
> +				size_t len)
> +{
> +	if (len < THC_IPTS_DESC_PREAMBLE)
> +		return;
> +
> +	len = min(len - THC_IPTS_DESC_PREAMBLE, idev->max_rx);
> +	memcpy(idev->desc, data + THC_IPTS_DESC_PREAMBLE, len);
> +	idev->desc_len = len;
> +	WRITE_ONCE(idev->have_desc, true);
> +	wake_up(&idev->wq);
> +}

[Severity: Medium]
Can a delayed retry response in thc_ipts_descriptor() overwrite idev->desc
and idev->desc_len while thc_ipts_hid_parse() is passing them to
hid_parse_report()?

Unlike thc_ipts_feature(), thc_ipts_descriptor() does not check a pending
flag or whether idev->have_desc is already set. If the first attempt in
thc_ipts_hid_parse() times out and a second THC_IPTS_TX_GET_DESC request is
sent, the delayed reply to the first attempt wakes thc_ipts_hid_parse(),
while the second reply on the IRQ thread races with hid_parse_report():

CPU0                                     CPU1 (thc_ipts_thread)
thc_ipts_hid_parse()
  try 0: thc_ipts_send(GET_DESC)
  wait_event_timeout() -> times out
  try 1: thc_ipts_send(GET_DESC)
                                         thc_ipts_descriptor() (reply 0)
                                           WRITE_ONCE(idev->have_desc, true)
                                           wake_up(&idev->wq)
  hid_parse_report(hdev, idev->desc,
                   idev->desc_len)
                                         thc_ipts_descriptor() (reply 1)
                                           memcpy(idev->desc, ...)
                                           idev->desc_len = len;
                                             <-- data race

Also, checking len < THC_IPTS_DESC_PREAMBLE allows an 8-byte preamble-only
frame to set idev->desc_len = 0; hid_parse_report() then sets
hdev->dev_rdesc = ZERO_SIZE_PTR via kmemdup() and registers a broken HID
device with a 0-byte report descriptor instead of retrying.

> +
> +/* The report is collected in a private buffer; the caller copies it out. */
> +static void thc_ipts_feature(struct thc_ipts *idev, const u8 *data,
> +			     size_t len)
> +{
> +	if (!READ_ONCE(idev->feature_pending))
> +		return;
> +
> +	len = min(len, idev->max_rx);
> +	memcpy(idev->feature, data, len);
> +	idev->feature_len = len;
> +	WRITE_ONCE(idev->feature_pending, false);
> +	WRITE_ONCE(idev->feature_done, true);
> +	wake_up(&idev->wq);
> +}
> +
>  static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
>  {
> +	const u8 *payload = idev->rx_frame + THC_IPTS_RX_HDR_SIZE;
>  	size_t capacity = idev->max_rx - THC_IPTS_RX_HDR_SIZE;
>  	u32 code, size;
>  
> @@ -145,11 +201,28 @@ static void thc_ipts_handle_frame(struct thc_ipts *idev, size_t len)
>  
>  	code = get_unaligned_le32(idev->rx_frame);
>  	/* The header may claim more than what actually arrived. */
> -	size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4),
> -		     capacity);
> +	size = min_t(u32, get_unaligned_le32(idev->rx_frame + 4), capacity);
>  	size = min_t(u32, size, len - THC_IPTS_RX_HDR_SIZE);
>  
> -	dev_dbg(idev->dev, "frame code %u, %u bytes\n", code, size);
> +	switch (code) {
> +	case THC_IPTS_RX_INPUT:
> +		if (idev->hid)
> +			hid_safe_input_report(idev->hid, HID_INPUT_REPORT, (u8 *)payload,
> +					      capacity, size, 1);
> +		break;
> +	case THC_IPTS_RX_DESC:
> +		thc_ipts_descriptor(idev, payload, size);
> +		break;
> +	case THC_IPTS_RX_FEATURE:
> +		thc_ipts_feature(idev, payload, size);
> +		break;

[Severity: High]
Can thc_ipts_handle_frame() be called with an uninitialized len and
redispatch a stale idev->rx_frame buffer when the RxDMA2 ring is already
empty?

In thc_ipts_read_frames(), len is not initialized before calling
thc_rxdma_read(), and thc_ipts_handle_frame(idev, len) runs before checking
finished:

drivers/hid/intel-thc-hid/intel-ipts/ipts.c:thc_ipts_read_frames() {
	for (;;) {
		int finished = 0;
		size_t len;

		if (thc_rxdma_read(idev->thc, THC_RXDMA2, idev->rx_frame,
				   &len, &finished)) {
...
		}

		thc_ipts_handle_frame(idev, len);
		if (finished)
			break;
	}
}

In thc_dma_read(), if is_dma_buf_empty() is true on entry, *read_len is
left untouched while *read_finished is set to 1 and the function returns 0:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:thc_dma_read() {
	if (!is_dma_buf_empty(dev, read_config, &read_ptr, &write_ptr)) {
		...
		*read_len = status;
		...
	}

	if (read_finished)
		*read_finished = is_dma_buf_empty(dev, read_config, &read_ptr, &write_ptr) ? 1 : 0;

	return 0;
}

If a second frame arrives after pass 0 of thc_ipts_thread() clears the
RxDMA2 EOF interrupt status bit in thc_interrupt_handler() and before pass
0's thc_ipts_read_frames() loop finishes draining the ring, pass 0 consumes
both frames, but the newly latched status bit causes pass 1 to call
thc_ipts_read_frames() on an empty ring. thc_rxdma_read() then returns 0
without setting len, and thc_ipts_handle_frame() uses the uninitialized
stack value to reprocess the previous frame in idev->rx_frame.

[ ... ]

> @@ -291,6 +378,175 @@ irqreturn_t thc_ipts_thread(int irq, void *data)
[ ... ]
> +/*
> + * A feature report is not pushed by the controller: it is asked for and comes
> + * back as a frame of its own, so the request waits for the handler.
> + */
> +static int thc_ipts_get_feature(struct thc_ipts *idev, unsigned char reportnum,
> +				u8 *buf, size_t len)
> +{
> +	int ret = -ETIMEDOUT;
> +	int try;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	WRITE_ONCE(idev->feature_done, false);
> +	WRITE_ONCE(idev->feature_pending, true);
> +
> +	for (try = 0; try < THC_IPTS_REQ_TRIES; try++) {
> +		ret = thc_ipts_send(idev, THC_IPTS_TX_GET_FEATURE, &reportnum, 1);
> +		if (ret)
> +			break;
> +
> +		if (wait_event_timeout(idev->wq, READ_ONCE(idev->feature_done),
> +				       msecs_to_jiffies(THC_IPTS_REQ_TIMEOUT_MS))) {
> +			ret = 0;
> +			break;
> +		}
> +
> +		ret = -ETIMEDOUT;
> +	}
> +
> +	WRITE_ONCE(idev->feature_pending, false);
> +
> +	if (ret) {
> +		dev_err(idev->dev, "controller did not return feature %u in %d tries\n",
> +			reportnum, try);
> +		return ret;
> +	}
> +
> +	/* The payload is the report as user space writes it, report ID first. */
> +	len = min(len, idev->feature_len);
> +	memcpy(buf, idev->feature, len);
> +
> +	return len;
> +}

[Severity: Medium]
Does thc_ipts_feature() need to verify that the incoming payload is
non-empty and starts with the requested reportnum?

If try 0 times out after 300 ms and try 1 sends a second request, a delayed
reply to try 0 completes try 1 while the second request remains in flight.
On a subsequent thc_ipts_get_feature() call for a different report ID, the
late reply from the earlier request is accepted by thc_ipts_feature() and
returned to the caller as if it matched the new reportnum. Also, if a
THC_IPTS_RX_FEATURE frame arrives with len == 0, thc_ipts_get_feature()
returns 0 without writing anything to buf.

> +
> +static int thc_ipts_hid_raw_request(struct hid_device *hdev,
> +				    unsigned char reportnum, __u8 *buf,
> +				    size_t len, unsigned char rtype, int reqtype)
> +{
> +	struct thc_ipts *idev = hdev->driver_data;
> +
> +	if (!buf || !len)
> +		return -EINVAL;
> +
> +	if (reqtype == HID_REQ_GET_REPORT && rtype == HID_FEATURE_REPORT)
> +		return thc_ipts_get_feature(idev, reportnum, buf, len);
> +
> +	if (reqtype != HID_REQ_SET_REPORT ||
> +	    (rtype != HID_OUTPUT_REPORT && rtype != HID_FEATURE_REPORT))
> +		return -EOPNOTSUPP;
> +
> +	guard(mutex)(&idev->req_lock);
> +
> +	if (rtype == HID_OUTPUT_REPORT)
> +		return thc_ipts_send(idev, THC_IPTS_TX_OUTPUT, buf, len);
> +
> +	return thc_ipts_send(idev, THC_IPTS_TX_SET_FEATURE, buf, len);
> +}

[Severity: High]
Should thc_ipts_hid_raw_request() return len on a successful
HID_REQ_SET_REPORT instead of the 0 returned by thc_ipts_send()?

thc_ipts_send() returns the result of thc_dma_write(), which is 0 on
success. However, struct hid_ll_driver.raw_request must return the number
of transferred bytes on success. In hidraw_send_report(), the return value
of __hid_hw_raw_request() is returned directly to user space for write()
and for HIDIOCSFEATURE and HIDIOCSOUTPUT ioctls, so returning 0 causes user
space (such as the daemon sending the report 5 set-feature request) to see
0 bytes transferred.

[ ... ]

> @@ -413,6 +669,8 @@ void thc_ipts_port_stop(struct thc_ipts *idev)
>  	cancel_work_sync(&idev->recover_work);
>  	thc_ipts_dma_stop(idev);
>  
> +	thc_ipts_hid_remove(idev);
> +
>  	if (idev->running)
>  		thc_ipts_touch_set(idev, false);

[Severity: High]
Can stopping DMA before removing the HID device in thc_ipts_port_stop()
cause a NULL pointer dereference or use-after-free in thc_dma_write()?

Until thc_ipts_hid_remove() destroys idev->hid, user space (via hidraw) or
a HID driver can still invoke thc_ipts_hid_raw_request() -> thc_ipts_send().
Because thc_ipts_dma_stop() does not take idev->req_lock or
idev->thc->thc_bus_lock, it can free the DMA scatterlists and PRD tables
while thc_ipts_send() is entering thc_dma_write():

CPU0                                     CPU1
thc_ipts_hid_raw_request()
  guard(mutex)(&idev->req_lock)
  thc_ipts_send()
    if (!idev->dma_ready) (true)
                                         thc_ipts_port_stop()
                                           thc_interrupt_enable(..., false)
                                           ...
                                           thc_ipts_dma_stop()
                                             idev->dma_ready = false
                                             thc_dma_unconfigure(idev->thc)
                                             thc_dma_release(idev->thc)
                                               sgl_free(config->sgls[0])
                                               config->sgls[0] = NULL
                                               config->prd_tbls = NULL
    thc_dma_write()
      write_dma_buffer()
        sg_copy_from_buffer(write_config->sgls[0], ...)
          <-- NULL pointer dereference / use-after-free

In addition, masking interrupts before thc_ipts_hid_remove() drains active
callers causes any in-flight thc_dma_write(), thc_ipts_get_feature(), or
thc_ipts_hid_parse() to stall until its timeout expires. Calling
thc_ipts_hid_remove() before disabling interrupts and stopping DMA avoids
both issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114154611.1683519-1-emfox@debian.org?part=3