Re: [PATCH 2/2] HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Multiple critical flaws in ACPI evaluation in `quicki2c_acpi_get_dsd_property` lead to memory leaks, potential stack buffer overflows, and type confusion.
- [High] Structs `i2c_config` and `i2c_param` are declared on the stack without zero-initialization, leading to uninitialized kernel memory being used for hardware configuration if the ACPI buffer is smaller than expected.
--

--- Patch [2]: [PATCH 2/2] HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters ---
commit 1910fa4a54d63973a7e0cc155b418558fa7c3450
Author: Even Xu <[email protected]>

HID: Intel-thc-hid: Intel-quicki2c: Support full I2C BUS config parameters

This patch reads complete I2C bus configuration parameters from ACPI and
passes them to thc_i2c_subip_init() to properly initialize the THC I2C
subip. This enhances hardware compatibility by allowing full
platform-specific I2C bus configurations.

> 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 46d3e9a019995..2ec52cb35a132 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -175,7 +175,9 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
>  	if (i2c_param.addressing_mode != HIDI2C_ADDRESSING_MODE_7BIT)
>  		return -EOPNOTSUPP;
>  
> -	qcdev->i2c_slave_addr = i2c_param.device_address;
> +	qcdev->i2c_config.addr_mode = HIDI2C_ADDRESSING_MODE_7BIT;
> +
> +	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);

[Severity: Critical]
This is a pre-existing issue, but does quicki2c_acpi_get_dsd_property() have
multiple flaws when evaluating ACPI methods?

When called here, quicki2c_acpi_get_dsd_property() receives an acpi_buffer, but
it ignores the passed type parameter and unconditionally assumes
ret_obj->type == ACPI_TYPE_BUFFER. If the ACPI method returns a different type,
does type confusion occur, causing ret_obj->buffer.pointer to resolve to
invalid memory and crash during memcpy()?

Additionally, there appears to be no bounds checking between the returned
ret_obj->buffer.length and the size of the destination stack buffer. Can an
ACPI table overflow the kernel stack here? Finally, is buffer.pointer leaked
before the function returns?

>  	if (ret)
>  		return ret;
>  
>  	if (i2c_param.connection_speed > 0 &&
>  	    i2c_param.connection_speed <= QUICKI2C_SUBIP_STANDARD_MODE_MAX_SPEED) {
> -		qcdev->i2c_speed_mode = THC_I2C_STANDARD;
> -		qcdev->i2c_clock_hcnt = i2c_config.SMHX;
> -		qcdev->i2c_clock_lcnt = i2c_config.SMLX;
> +		qcdev->i2c_config.speed = THC_I2C_STANDARD;
> +		qcdev->i2c_config.scl_hcnt = (u32)i2c_config.SMHX;
> +		qcdev->i2c_config.scl_lcnt = (u32)i2c_config.SMLX;
> +		qcdev->i2c_config.sda_tx_hold = (u32)i2c_config.SMTD;
> +		qcdev->i2c_config.sda_rx_hold = (u32)i2c_config.SMRD;

[Severity: High]
This isn't a bug introduced by this patch, but are the i2c_config and
i2c_param structures declared on the stack in quicki2c_get_acpi_resources()
without zero-initialization?

If the ACPI firmware returns a shorter buffer, quicki2c_acpi_get_dsd_property()
will only populate part of the structures. This means the newly added code
above will read uninitialized stack memory for fields like SMTD and SMRD.
Can this lead to invalid bus timings and system instability?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2