RE: [PATCH v2] HID: intel-thc-hid: validate ACPI buffer length before memcpy

[Xu, Even]

> -----Original Message-----
> From: Tristan Madani <tristmd@xxxxxxxxx>
> Sent: Sunday, October 25, 2026 11:48 PM
> To: Xu, Even <even.xu@xxxxxxxxx>; Sun, Xinpeng <xinpeng.sun@xxxxxxxxx>;
> jikos@xxxxxxxxxx; bentiss@xxxxxxxxxx
> Cc: srinivas.pandruvada@xxxxxxxxxxxxxxx; mpearson-lenovo@xxxxxxxxx; linux-
> input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Tristan Madani
> <tristan@xxxxxxxxxxxxxxxxxxx>; stable@xxxxxxxxxxxxxxx
> Subject: [PATCH v2] HID: intel-thc-hid: validate ACPI buffer length before memcpy
> 
> From: Tristan Madani <tristan@xxxxxxxxxxxxxxxxxxx>
> 
> The QuickI2C and QuickSPI ACPI helper functions copy firmware-returned buffers
> into caller-provided structures without verifying the buffer length. Malformed or
> short ACPI tables could cause an out-of-bounds read from the firmware buffer,
> and oversized responses could overflow the destination struct.
> 
> Additionally, quicki2c_acpi_get_dsd_property() does not validate the returned
> ACPI object type before accessing buffer fields, and does not free the allocated
> ACPI buffer on the success path, leaking memory on every call.
> 
> Add a buf_size parameter to the ACPI helper functions and reject responses
> larger than the destination buffer. Accept shorter responses to handle firmware
> that returns buffers without struct tail padding (e.g. ISUB tables returning 177 or
> 145 bytes for a 184-byte struct).
> Zero-initialize caller stack structures so fields beyond the firmware response
> length are deterministic. Add the missing type check and kfree to the DSD helper.
> 
> Fixes: 5282e45ccbfa ("HID: intel-thc-hid: intel-quicki2c: Add THC QuickI2C ACPI
> interfaces")
> Fixes: 7cb06f08ef13 ("HID: intel-thc-hid: intel-quickspi: Add THC QuickSPI ACPI
> interfaces")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Tristan Madani <tristan@xxxxxxxxxxxxxxxxxxx>
> ---
> Changes in v2:
> - Use overflow-only check (>) instead of exact-match (!=) so firmware
>   returning shorter buffers (e.g. 177 or 145 bytes for 184-byte ISUB
>   struct, or 1 byte for 4-byte SPI opcode) is accepted
> - Zero-initialize caller stack structures for deterministic behavior
>   with short firmware responses
> - Add missing type validation and kfree in DSD helper
> - Add braces per coding style
> 
>  .../intel-quicki2c/pci-quicki2c.c             | 40 ++++++++++++++-----
>  .../intel-quickspi/pci-quickspi.c             | 36 ++++++++++-------
>  2 files changed, 51 insertions(+), 25 deletions(-)
> 
> 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 7a913f717efaad..dd21864d16b05b 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -62,7 +62,8 @@ static const struct acpi_gpio_mapping quicki2c_gpios[] = {
>   * Return: 0 if success or ENODEV on failure.
>   */
>  static int quicki2c_acpi_get_dsm_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;
> @@ -75,10 +76,15 @@ static int quicki2c_acpi_get_dsm_property(struct
> acpi_device *adev, const guid_t
>  		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) {
> +			ACPI_FREE(obj);
> +			return -ENOSPC;
> +		}
>  		memcpy(prop_buf, obj->buffer.pointer, obj->buffer.length);
> +	}
> 
>  	ACPI_FREE(obj);
> 
> @@ -97,7 +103,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 +120,16 @@ 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) {
> +		kfree(buffer.pointer);
> +		return -ENOSPC;
> +	}
>  	memcpy(prop_buf, ret_obj->buffer.pointer, ret_obj->buffer.length);
> +	kfree(buffer.pointer);
> 
>  	return 0;
>  }
> @@ -129,8 +145,8 @@ static int quicki2c_acpi_get_dsd_property(struct
> acpi_device *adev, acpi_string  static int quicki2c_get_acpi_resources(struct
> quicki2c_device *qcdev)  {
>  	struct acpi_device *adev = ACPI_COMPANION(qcdev->dev);
> -	struct quicki2c_subip_acpi_parameter i2c_param;
> -	struct quicki2c_subip_acpi_config i2c_config;
> +	struct quicki2c_subip_acpi_parameter i2c_param = {};
> +	struct quicki2c_subip_acpi_config i2c_config = {};
>  	u32 hid_desc_addr;
>  	int ret = -EINVAL;
> 
> @@ -145,7 +161,7 @@ static int quicki2c_get_acpi_resources(struct
> quicki2c_device *qcdev)
>  					     QUICKI2C_ACPI_REVISION_NUM,
> 
> QUICKI2C_ACPI_FUNC_NUM_HID_DESC_ADDR,
>  					     ACPI_TYPE_INTEGER,
> -					     &hid_desc_addr);
> +					     &hid_desc_addr, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -155,7 +171,7 @@ static int quicki2c_get_acpi_resources(struct
> quicki2c_device *qcdev)
>  					     QUICKI2C_ACPI_REVISION_NUM,
> 
> QUICKI2C_ACPI_FUNC_NUM_ACTIVE_LTR_VAL,
>  					     ACPI_TYPE_INTEGER,
> -					     &qcdev->active_ltr_val);
> +					     &qcdev->active_ltr_val, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -163,12 +179,13 @@ static int quicki2c_get_acpi_resources(struct
> quicki2c_device *qcdev)
>  					     QUICKI2C_ACPI_REVISION_NUM,
> 
> QUICKI2C_ACPI_FUNC_NUM_LP_LTR_VAL,
>  					     ACPI_TYPE_INTEGER,
> -					     &qcdev->low_power_ltr_val);
> +					     &qcdev->low_power_ltr_val, 0);
>  	if (ret)
>  		return ret;
> 
>  	ret = quicki2c_acpi_get_dsd_property(adev,
> QUICKI2C_ACPI_METHOD_NAME_ICRS,
> -					     ACPI_TYPE_BUFFER, &i2c_param);
> +					     ACPI_TYPE_BUFFER, &i2c_param,
> +					     sizeof(i2c_param));
>  	if (ret)
>  		return ret;
> 
> @@ -180,7 +197,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;
> 
> 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 53023e30cf8d47..e4331ba4e503d9 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,15 @@ 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) {
> +			ACPI_FREE(obj);
> +			return -ENOSPC;
> +		}
>  		memcpy(prop_buf, obj->buffer.pointer, obj->buffer.length);
> +	}
> 
>  	ACPI_FREE(obj);
> 
> @@ -129,7 +135,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_INPUT_REP_HDR_ADDR,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->input_report_hdr_addr);
> +				    &qsdev->input_report_hdr_addr, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -137,7 +143,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_INPUT_REP_BDY_ADDR,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->input_report_bdy_addr);
> +				    &qsdev->input_report_bdy_addr, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -145,7 +151,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_OUTPUT_REP_ADDR,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->output_report_addr);
> +				    &qsdev->output_report_addr, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -153,7 +159,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 +168,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;
> 
> @@ -169,7 +177,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
>  				    ACPI_QUICKSPI_FUNC_NUM_IO_MODE,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->spi_read_io_mode);
> +				    &qsdev->spi_read_io_mode, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -184,7 +192,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_CONNECTION_SPEED,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->spi_freq_val);
> +				    &qsdev->spi_freq_val, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -192,7 +200,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_LIMIT_PACKET_SIZE,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->limit_packet_size);
> +				    &qsdev->limit_packet_size, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -205,7 +213,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
> 
> ACPI_QUICKSPI_FUNC_NUM_PERFORMANCE_LIMIT,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->performance_limit);
> +				    &qsdev->performance_limit, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -215,7 +223,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
>  				    ACPI_QUICKSPI_FUNC_NUM_ACTIVE_LTR,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->active_ltr_val);
> +				    &qsdev->active_ltr_val, 0);
>  	if (ret)
>  		return ret;
> 
> @@ -223,7 +231,7 @@ static int quickspi_get_acpi_resources(struct
> quickspi_device *qsdev)
>  				    ACPI_QUICKSPI_REVISION_NUM,
>  				    ACPI_QUICKSPI_FUNC_NUM_LP_LTR,
>  				    ACPI_TYPE_INTEGER,
> -				    &qsdev->low_power_ltr_val);
> +				    &qsdev->low_power_ltr_val, 0);
>  	if (ret)
>  		return ret;
> 

Thanks for your patch!
It will be better if you can split it into two separate patches (One for QuickSPI, one for QuickI2C).

Reviewed-by: Even Xu <even.xu@xxxxxxxxx>

> --
> 2.47.3