Re: [PATCH] HID: intel-thc-hid: validate ACPI buffer length before use

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] HID: intel-thc-hid: ISUB size check in quicki2c_acpi_get_dsd_property() fails
- [High] HID: intel-thc-hid: opcode size check in thc_acpi_get_property() fails probe
--

commit 7818ecd95e7c011bb989cd8c07a2ef782c095de2
Author: Tristan Madani <tristan@talencesecurity.com>

HID: intel-thc-hid: validate ACPI buffer length before use

This commit adds a buf_size parameter to the QuickI2C and QuickSPI ACPI
property helpers to enforce exact buffer length matching for buffer properties
and adds object type validation to the QuickI2C DSD helper.

> 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 7a913f717efaa..6f70b64f20105 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c

[ ... ]

> @@ -97,7 +105,8 @@ static int quicki2c_acpi_get_dsm_property(struct acpi_device *adev, const guid_t
>   * Return: 0 if success or ENODEV on failed.
>   */
>  static int quicki2c_acpi_get_dsd_property(struct acpi_device *adev, acpi_string dsd_method_name,
> -					  acpi_object_type type, void *prop_buf)
> +					  acpi_object_type type, void *prop_buf,
> +					  size_t buf_size)
>  {
>  	acpi_handle handle = acpi_device_handle(adev);
>  	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
> @@ -113,7 +122,18 @@ static int quicki2c_acpi_get_dsd_property(struct acpi_device *adev, acpi_string
>  
>  	ret_obj = buffer.pointer;
>  
> +	if (ret_obj->type != type) {
> +		kfree(buffer.pointer);
> +		return -EINVAL;
> +	}
> +	if (buf_size > 0 && ret_obj->buffer.length != buf_size) {
> +		int err = ret_obj->buffer.length > buf_size ? -ENOSPC : -EINVAL;
> +
> +		kfree(buffer.pointer);
> +		return err;
> +	}
>  	memcpy(prop_buf, ret_obj->buffer.pointer, ret_obj->buffer.length);
> +	kfree(buffer.pointer);
>  
>  	return 0;
>  }

[ ... ]

> @@ -180,7 +201,8 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
>  	qcdev->i2c_config.target_addr = i2c_param.device_address;
>  
>  	ret = quicki2c_acpi_get_dsd_property(adev, QUICKI2C_ACPI_METHOD_NAME_ISUB,
> -					     ACPI_TYPE_BUFFER, &i2c_config);
> +					     ACPI_TYPE_BUFFER, &i2c_config,
> +					     sizeof(i2c_config));
>  	if (ret)
>  		return ret;

[Severity: High]
Will requiring ret_obj->buffer.length to equal sizeof(i2c_config) in
quicki2c_get_acpi_resources() -> quicki2c_acpi_get_dsd_property() cause
QuickI2C probing to fail with -EINVAL?

In drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-dev.h,
struct quicki2c_subip_acpi_config is defined without __packed:

struct quicki2c_subip_acpi_config {
	u64 SMHX;
	...
	u64 FSEN;
	u64 FSVL;
	u64 INDE;
	u64 INDV;
	u8 reserved;
};

With 22 u64 fields (176 bytes) followed by u8 reserved (1 byte), the compiler
adds 7 bytes of tail alignment padding on 64-bit architectures, making
sizeof(i2c_config) equal to 184 bytes.

Firmware ACPI buffers returned by the ISUB method do not include compiler tail
padding. Tables with the 4 advanced-feature u64 fields (FSEN, FSVL, INDE,
INDV, added in commit 0b1fca9dce44 ("HID: intel-thc-hid: intel-quicki2c:
Change ISUB table length")) are at most 177 bytes (0xB1), while Lunar Lake and
older BIOSes (as noted in commit 1db9df89a213 ("HID: intel-thc-hid:
intel-quicki2c: Update ACPI ISUB method support")) return 145 bytes (0x91).

Because ret_obj->buffer.length != buf_size rejects anything other than 184
bytes, won't every valid ISUB table be rejected? Also, if shorter 145-byte
ISUB buffers are accepted when qcdev->ddata is non-NULL, should i2c_config be
zero-initialized in quicki2c_get_acpi_resources() so that FSEN, FSVL, INDE,
and INDV are not read uninitialized from the stack?

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index 53023e30cf8d4..29689f41f6bf0 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -81,7 +81,8 @@ static const struct acpi_gpio_mapping quickspi_gpios[] = {
>   * Return: 0 if successful or ENODEV on failed.
>   */
>  static int thc_acpi_get_property(struct acpi_device *adev, const guid_t *guid,
> -				 u64 rev, u64 func, acpi_object_type type, void *prop_buf)
> +				 u64 rev, u64 func, acpi_object_type type,
> +				 void *prop_buf, size_t buf_size)
>  {
>  	acpi_handle handle = acpi_device_handle(adev);
>  	union acpi_object *obj;
> @@ -94,10 +95,20 @@ static int thc_acpi_get_property(struct acpi_device *adev, const guid_t *guid,
>  		return -ENODEV;
>  	}
>  
> -	if (type == ACPI_TYPE_INTEGER)
> +	if (type == ACPI_TYPE_INTEGER) {
>  		*(u32 *)prop_buf = (u32)obj->integer.value;
> -	else if (type == ACPI_TYPE_BUFFER)
> +	} else if (type == ACPI_TYPE_BUFFER) {
> +		if (buf_size > 0 && obj->buffer.length != buf_size) {
> +			int err = obj->buffer.length > buf_size ? -ENOSPC : -EINVAL;
> +
> +			acpi_handle_err(handle,
> +					"_DSM buffer size mismatch: %llu vs %zu\n",
> +					(unsigned long long)obj->buffer.length, buf_size);
> +			ACPI_FREE(obj);
> +			return err;
> +		}
>  		memcpy(prop_buf, obj->buffer.pointer, obj->buffer.length);
> +	}
>  
>  	ACPI_FREE(obj);
>  
> @@ -153,7 +164,8 @@ static int quickspi_get_acpi_resources(struct quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
>  				    ACPI_QUICKSPI_FUNC_NUM_READ_OPCODE,
>  				    ACPI_TYPE_BUFFER,
> -				    &qsdev->spi_read_opcode);
> +				    &qsdev->spi_read_opcode,
> +				    sizeof(qsdev->spi_read_opcode));
>  	if (ret)
>  		return ret;
>  
> @@ -161,7 +173,8 @@ static int quickspi_get_acpi_resources(struct quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
>  				    ACPI_QUICKSPI_FUNC_NUM_WRITE_OPCODE,
>  				    ACPI_TYPE_BUFFER,
> -				    &qsdev->spi_write_opcode);
> +				    &qsdev->spi_write_opcode,
> +				    sizeof(qsdev->spi_write_opcode));
>  	if (ret)
>  		return ret;

[Severity: High]
Could passing sizeof(qsdev->spi_read_opcode) and
sizeof(qsdev->spi_write_opcode) to thc_acpi_get_property() break probing when
firmware returns 1-byte SPI opcode buffers?

In drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h, spi_read_opcode
and spi_write_opcode in struct quickspi_device are u32 (4 bytes):

	u32 spi_read_opcode;
	u32 spi_write_opcode;

However, per the HIDSPI _DSM specification (functions 4 and 5) and the
register configuration in drivers/hid/intel-thc-hid/intel-thc/intel-thc-dev.c
in thc_spi_read_config() and thc_spi_write_config():

	if (io_mode == THC_QUAD_IO)
		opcode = FIELD_PREP(THC_M_PRT_SPI_ICRRD_OPCODE_SPI_QIO, opcode);
	else if (io_mode == THC_DUAL_IO)
		opcode = FIELD_PREP(THC_M_PRT_SPI_ICRRD_OPCODE_SPI_DIO, opcode);
	else
		opcode = FIELD_PREP(THC_M_PRT_SPI_ICRRD_OPCODE_SPI_SIO, opcode);

each SPI opcode is a single byte returned as a 1-byte ACPI buffer
(obj->buffer.length == 1) that was previously copied into the low byte of the
zero-allocated u32 field. With obj->buffer.length != buf_size checking against
4, won't thc_acpi_get_property() reject 1-byte opcode buffers with -EINVAL?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024205850.4051206-1-tristmd@gmail.com?part=1