Re: [PATCH 1/4] HID: intel-quicki2c: Use min() macro to simplify code

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the helper function `quicki2c_acpi_get_dsd_property()` fails to validate the ACPI object type, performs an unbounded memory copy, and leaks the dynamically allocated ACPI buffer.
--

--- Patch [1]: [PATCH 1/4] HID: intel-quicki2c: Use min() macro to simplify code ---
commit 232da0a8d999e7cca061120fe2a92fd5ce93fd4e
Author: Chen Changcheng <[email protected]>
Subject: HID: intel-quicki2c: Use min() macro to simplify code

This patch replaces open-coded min() logic with the standard min() macro in
pci-quicki2c.c. This is used for setting i2c_max_frame_size and i2c_int_delay
to improve code readability and maintain consistency with kernel style.

> 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..bd099600bb9f2 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c
> @@ -210,15 +210,8 @@ static int quicki2c_get_acpi_resources(struct quicki2c_device *qcdev)
>  		qcdev->i2c_max_frame_size_enable = i2c_config.FSEN;
>  		qcdev->i2c_int_delay_enable = i2c_config.INDE;
>  

[Severity: High]
This isn't a bug introduced by this patch, but since this code relies on 
i2c_config populated earlier by quicki2c_acpi_get_dsd_property(), does the 
helper function validate the ACPI object type before copying it?

Looking at quicki2c_acpi_get_dsd_property():

quicki2c_acpi_get_dsd_property(...)
{
        ...
        memcpy(prop_buf, ret_obj->buffer.pointer, ret_obj->buffer.length);
        ...
        return 0;
}

Does this memcpy blindly use the length from the ACPI object without
ensuring it fits into the destination buffer (such as the stack-allocated
i2c_config)? If firmware provides a larger buffer, could this overflow the
stack frame in quicki2c_get_acpi_resources()?

Also, if ret_obj->type is not verified as a buffer, could this cause an
invalid pointer dereference since the buffer pointer might overlap with
an integer value?

Additionally, does the success path in quicki2c_acpi_get_dsd_property()
leak the dynamically allocated ACPI buffer by returning without calling
ACPI_FREE(buffer.pointer)?

> -		if (i2c_config.FSVL <= qcdev->ddata->max_detect_size)
> -			qcdev->i2c_max_frame_size = i2c_config.FSVL;
> -		else
> -			qcdev->i2c_max_frame_size = qcdev->ddata->max_detect_size;
> -
> -		if (i2c_config.INDV <= qcdev->ddata->max_interrupt_delay)
> -			qcdev->i2c_int_delay = i2c_config.INDV;
> -		else
> -			qcdev->i2c_int_delay = qcdev->ddata->max_interrupt_delay;
> +		qcdev->i2c_max_frame_size = min(i2c_config.FSVL, qcdev->ddata->max_detect_size);
> +		qcdev->i2c_int_delay = min(i2c_config.INDV, qcdev->ddata->max_interrupt_delay);
>  	}
>  
>  	return 0;

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