Re: [PATCH AUTOSEL 6.18] HID: Intel-thc-hid: Intel-quicki2c: Fix buffer overflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] HID: quicki2c: input_buf size mismatch with DMA in quicki2c_alloc_report_buf()
- [Medium] HID: intel-thc-hid: incomplete report_buf sizing in quicki2c_alloc_report_buf()
--

commit 916fd20197b9aebc1d04d169b7d6a4523c3d0a4d
Author: Even Xu <even.xu@intel.com>

HID: Intel-thc-hid: Intel-quicki2c: Fix buffer overflow

This commit enlarges the qcdev->input_buf and qcdev->report_buf allocations
in quicki2c_alloc_report_buf() by wrapping max_report_len with
HIDI2C_PACKET_LEN() to account for the 2-byte HID-over-I2C packet length
header.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> index c6ba6886072ad..4588c55cf255d 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -586,7 +586,8 @@ static int quicki2c_alloc_report_buf(struct quicki2c_device *qcdev)
>  	 */
>  	max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len), SZ_4K);
>  
> -	qcdev->input_buf = devm_kzalloc(qcdev->dev, max_report_len, GFP_KERNEL);
> +	qcdev->input_buf = devm_kzalloc(qcdev->dev,
> +					HIDI2C_PACKET_LEN(max_report_len), GFP_KERNEL);

[Severity: High]
Can qcdev->input_buf still be overflowed or mismatched with the DMA packet
limits when reading reports into it?

In drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c, read_dma_buffer()
bounds-checks the incoming DMA transfer only against
read_config->max_packet_size before copying into read_buff
(qcdev->input_buf):

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:read_dma_buffer() {
	prd_tbl = &read_config->prd_tbls[prd_table_index];
	mes_len = calc_message_len(prd_tbl, &nent);
	if (mes_len > read_config->max_packet_size) {
		...
		return -EMSGSIZE;
	}

	sg = read_config->sgls[prd_table_index];
	ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);
}

Meanwhile, quicki2c_dma_init() and quicki2c_dma_adv_enable() in
drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c configure the DMA
and hardware frame limits using the raw max_input_len and report_desc_len
without HIDI2C_PACKET_LEN():

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_dma_init() {
	swdma_max_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len),
			    le16_to_cpu(qcdev->dev_desc.report_desc_len));

	ret = thc_dma_set_max_packet_sizes(qcdev->thc_hw, 0,
					   le16_to_cpu(qcdev->dev_desc.max_input_len),
					   le16_to_cpu(qcdev->dev_desc.max_output_len),
					   swdma_max_len);
}

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_dma_adv_enable() {
	if (qcdev->i2c_max_frame_size_enable) {
		if (qcdev->i2c_max_frame_size >=
		    le16_to_cpu(qcdev->dev_desc.max_input_len)) {
			thc_i2c_set_rx_max_size(qcdev->thc_hw,
						le16_to_cpu(qcdev->dev_desc.max_input_len));
}

Because dma_set_max_packet_size() rounds max_packet_size up with
ALIGN(size, SZ_4K), this leaves three mismatches on the
quicki2c_irq_thread_handler() -> handle_input_report() -> thc_rxdma_read()
and quicki2c_hid_raw_request() -> quicki2c_get_report() -> thc_swdma_read()
paths:

1. If report_desc_len is larger than max(max_input_len, SZ_4K) + 2,
THC_SWDMA has max_packet_size = ALIGN(report_desc_len, SZ_4K) while
qcdev->input_buf is only max(max_input_len, SZ_4K) + 2 bytes, so
quicki2c_get_report() calling thc_swdma_read() with prd_tbl_len = NULL can
copy past the end of qcdev->input_buf.

2. If max_input_len > SZ_4K and is not within 2 bytes of a 4K multiple (for
example, 4100), qcdev->input_buf is 4102 bytes while THC_RXDMA2 and
THC_SWDMA have max_packet_size = 8192 bytes, allowing read_dma_buffer() to
overflow qcdev->input_buf if a larger DMA transfer arrives.

3. If max_input_len is 4095 or an exact multiple of SZ_4K (such as 4096),
passing raw max_input_len to thc_dma_set_max_packet_sizes() and
thc_i2c_set_rx_max_size() leaves the DMA buffer and RX max frame size at
4096 or 4095 bytes, whereas a full-size input packet with its 2-byte length
header requires 4098 or 4097 bytes.

Should quicki2c_dma_init(), quicki2c_dma_adv_enable(), and the
qcdev->input_buf allocation all use consistent packet sizes?

>  	if (!qcdev->input_buf)
>  		return -ENOMEM;
>  
> @@ -596,7 +597,8 @@ static int quicki2c_alloc_report_buf(struct quicki2c_device *qcdev)
>  	max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_output_len),
>  			     max_report_len);
>  
> -	qcdev->report_buf = devm_kzalloc(qcdev->dev, max_report_len, GFP_KERNEL);
> +	qcdev->report_buf = devm_kzalloc(qcdev->dev,
> +					 HIDI2C_PACKET_LEN(max_report_len), GFP_KERNEL);
>  	if (!qcdev->report_buf)
>  		return -ENOMEM;
>  

[Severity: Medium]
Does enlarging qcdev->report_buf by HIDI2C_PACKET_LEN(max_report_len) allow
maximum-length output or feature reports to succeed?

At the end of quicki2c_alloc_report_buf(), qcdev->report_len is still set
to max_report_len:

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_alloc_report_buf() {
	qcdev->report_buf = devm_kzalloc(qcdev->dev,
					 HIDI2C_PACKET_LEN(max_report_len), GFP_KERNEL);
	if (!qcdev->report_buf)
		return -ENOMEM;

	qcdev->report_len = max_report_len;
}

When write_cmd_to_txdma() or quicki2c_get_report() in
drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c formats a
packet into qcdev->report_buf, it passes qcdev->report_len as write_buf_len
to quicki2c_init_write_buf():

drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c:write_cmd_to_txdma() {
	ret = quicki2c_init_write_buf(qcdev, cmd, cmd_len, buf ? true : false, buf,
				      buf_len, qcdev->report_buf, qcdev->report_len);
}

And quicki2c_init_write_buf() prepends cmd_reg (HIDI2C_REG_LEN = 2 bytes),
cmd (HIDI2C_CMD_LEN = 2 bytes, or HIDI2C_CMD_LEN_OPT = 3 bytes when
report_id >= 15), data_reg (HIDI2C_REG_LEN = 2 bytes), and the packet
length header (HIDI2C_LENGTH_LEN = 2 bytes) before checking against
write_buf_len:

drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c:quicki2c_init_write_buf() {
	buf_len = HIDI2C_REG_LEN + cmd_len;

	if (append_data_reg)
		buf_len += HIDI2C_REG_LEN;

	if (data && data_len)
		buf_len += data_len + HIDI2C_LENGTH_LEN;

	if (buf_len > write_buf_len)
		return -EINVAL;
}

Because qcdev->report_len is left at max_report_len, the extra 2 bytes in
qcdev->report_buf cannot be used, and because quicki2c_init_write_buf()
writes 8 to 9 bytes of framing ahead of data_len, HIDI2C_PACKET_LEN() (only
2 extra bytes) is still 6 to 7 bytes short for a payload of max_report_len
bytes, causing quicki2c_init_write_buf() to return -EINVAL.

Should qcdev->report_buf and qcdev->report_len both be sized to include the
full command, register, and packet length headers written by
quicki2c_init_write_buf()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019135441.1420968-123-sashal@kernel.org?part=1