Re: [PATCH v5 5/6] HID: himax: Report the touch and heatmap data

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Heap buffer overflow in heatmap decompression due to integer truncation
- [High] Null pointer dereference in himax_decompress_heatmap due to concurrent IRQ execution
--

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

HID: himax: Report the touch and heatmap data

This commit adds reporting of heatmap data for the HX83102J firmware.
It extracts the heatmap following the stylus report and decompresses
it from 12-bit to 16-bit values.

Link: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/5326106

> diff --git a/drivers/hid/hid-himax.c b/drivers/hid/hid-himax.c
> index 533d7be629147..40abae724743d 100644
> --- a/drivers/hid/hid-himax.c
> +++ b/drivers/hid/hid-himax.c
> @@ -2150,6 +2150,37 @@ static void himax_hid_remove(struct himax_ts_data *ts)
>  	ts->hid = NULL;
>  }
>  
> +/**
> + * himax_decompress_heatmap() - Unpack the heatmap into its HID report
> + * @ts: Himax touch screen data
> + * @src: Heatmap as read from the event stack
> + *
> + * The IC packs two 12-bit heatmap values into three bytes, while the HID
> + * report descriptor of the firmware declares 16-bit values. Copy the report
> + * ID and heatmap info header, then widen every pair of values to the
> + * little-endian 16-bit layout of the report.
> + *
> + * Return: None
> + */
> +static void himax_decompress_heatmap(struct himax_ts_data *ts, const u8 *src)
> +{
> +	u32 i;
> +	u32 count = ts->ic_data.rx_num * ts->ic_data.tx_num;
> +	const u32 header_size = HIMAX_HEAT_MAP_INFO_SZ + 1;
> +	u8 *dst = ts->heatmap_buf;
> +
> +	memcpy(dst, src, header_size);

[Severity: High]
Does this code dereference a NULL pointer if the IRQ fires concurrently?

If an interrupt fires early, himax_ts_thread() executes himax_ts_operation()
which locklessly checks ts->hid_probed.

If another thread running himax_initial_work() sets ts->hid_probed = true via
himax_hid_register(), but hasn't yet allocated ts->heatmap_buf in
himax_hid_report_data_init(), won't dst be NULL here when memcpy() is called?

> +	for (i = 0; i < count; i += 2) {
> +		const u8 *packed = src + header_size + i * 3 / 2;
> +		u8 *unpacked = dst + header_size + i * 2;
> +
> +		unpacked[0] = packed[0];
> +		unpacked[1] = packed[2] >> 4;
> +		unpacked[2] = packed[1];
> +		unpacked[3] = packed[2] & 0x0f;
> +	}
> +}

[Severity: High]
Can this loop read and write past the end of the allocated buffers if count
is an odd number?

Since count is the product of rx_num and tx_num read dynamically from the
device, it might be odd. The loop processes elements in pairs (i += 2).

During the final iteration with an odd count, won't this write 4 bytes to
unpacked and read 3 bytes from packed, exceeding the bounds of both buffers?

[ ... ]

> @@ -2322,6 +2362,9 @@ static int himax_hid_report_data_init(struct himax_ts_data *ts)
>  	ts->touch_data_sz = ts->hid_desc.max_input_length;
>  	if (ts->ic_data.stylus_function)
>  		ts->touch_data_sz += ts->hid_desc.max_input_length;
> +	ts->heatmap_data_size = ts->ic_data.rx_num * ts->ic_data.tx_num * 3 / 2;

[Severity: High]
Does this calculation truncate the required size if the product of rx_num and
tx_num is odd?

If the count is odd, this division by 2 truncates the result, potentially
leading to an undersized allocation for xfer_buf that doesn't account for the
full packed heatmap size.

> +	ts->touch_data_sz += HIMAX_HEAT_MAP_HEADER_SZ +
> +			     HIMAX_HEAT_MAP_INFO_SZ + ts->heatmap_data_size;
>  	if (ts->touch_data_sz != ts->xfer_buf_sz) {
>  		kfree(ts->xfer_buf);
>  		ts->xfer_buf_sz = 0;
> @@ -2331,6 +2374,14 @@ static int himax_hid_report_data_init(struct himax_ts_data *ts)
>  		ts->xfer_buf_sz = ts->touch_data_sz;
>  	}
>  
> +	if (!ts->heatmap_buf) {
> +		ts->heatmap_buf = devm_kzalloc(ts->dev,
> +					       ts->ic_data.rx_num * ts->ic_data.tx_num * 2 +
> +					       HIMAX_HEAT_MAP_INFO_SZ + 1, GFP_KERNEL);

[Severity: High]
Does this allocation fail to account for the loop's 2-element processing
chunk size?

If the count is odd, the destination buffer is sized exactly for the odd count.
Since the decompression loop in himax_decompress_heatmap() processes elements
in pairs, won't it write past the end of this allocated buffer?

> +		if (!ts->heatmap_buf)
> +			return -ENOMEM;
> +	}
> +
>  	return 0;
>  }

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