Re: [PATCH v4 5/9] platform/x86: dell-ddv: Use new buffer-based WMI API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] WMI query wrapper functions crash when the underlying ACPI method returns no data (ZERO_SIZE_PTR dereference).
--

commit 9a4b15969c05e28f24a154f47da550f56ba01716
Author: Armin Wolf <W_Armin@gmx.de>

platform/x86: dell-ddv: Use new buffer-based WMI API
    
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI intergers/strings/packages
for exchanging data.

> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 62e3d060f0385..c2120d8ae2988 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
[ ... ]
> @@ -158,105 +164,96 @@ static const char * const fan_dock_labels[] = {
[ ... ]
>  static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
>  				      u32 arg, u32 *res)
>  {
> -	union acpi_object *obj;
> +	__le32 *argr __free(kfree) = NULL;
> +	struct wmi_buffer output;
>  	int ret;
>  
> -	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_INTEGER);
> +	ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(*argr));
>  	if (ret < 0)
>  		return ret;
>  
> -	if (obj->integer.value <= U32_MAX)
> -		*res = (u32)obj->integer.value;
> -	else
> -		ret = -ERANGE;
> -
> -	kfree(obj);
> +	argr = output.data;
> +	*res = le32_to_cpu(*argr);

[Severity: High]
Can this result in a ZERO_SIZE_PTR dereference?

If wmidev_invoke_method() evaluates the ACPI method successfully but it
returns no object, it returns 0 (success) and sets output.data to
ZERO_SIZE_PTR. Since this function only checks if ret < 0, does this
unconditionally dereference ZERO_SIZE_PTR if the firmware returns no data?

>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int dell_wmi_ddv_query_buffer(struct wmi_device *wdev, enum dell_ddv_method method,
> -				     u32 arg, union acpi_object **result)
> +				     u32 arg, struct dell_wmi_buffer **result)
>  {
> -	union acpi_object *obj;
> -	u64 buffer_size;
> +	struct dell_wmi_buffer *buffer;
> +	struct wmi_buffer output;
> +	size_t buffer_size;
>  	int ret;
>  
> -	ret = dell_wmi_ddv_query_type(wdev, method, arg, &obj, ACPI_TYPE_PACKAGE);
> +	ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(*buffer));
>  	if (ret < 0)
>  		return ret;
>  
> -	if (obj->package.count != 2 ||
> -	    obj->package.elements[0].type != ACPI_TYPE_INTEGER ||
> -	    obj->package.elements[1].type != ACPI_TYPE_BUFFER) {
> -		ret = -ENOMSG;
> -
> -		goto err_free;
> -	}
> -
> -	buffer_size = obj->package.elements[0].integer.value;
> -
> -	if (!buffer_size) {
> +	buffer = output.data;
> +	if (!le32_to_cpu(buffer->raw_size)) {

[Severity: High]
Similarly, does this dereference buffer->raw_size when buffer is ZERO_SIZE_PTR
if the underlying ACPI method returns no data?

[ ... ]
> -static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
> -				     u32 arg, union acpi_object **result)
> +static ssize_t dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_method method,
> +					 u32 arg, char *buf, size_t length)
>  {
> -	return dell_wmi_ddv_query_type(wdev, method, arg, result, ACPI_TYPE_STRING);
> +	struct wmi_string *str __free(kfree) = NULL;
> +	struct wmi_buffer output;
> +	size_t str_size;
> +	int ret;
> +
> +	ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(*str));
> +	if (ret < 0)
> +		return ret;
> +
> +	str = output.data;
> +	str_size = sizeof(*str) + le16_to_cpu(str->length);

[Severity: High]
Could str->length crash here for the same reason, if wmidev_invoke_method()
returns 0 but sets output.data to ZERO_SIZE_PTR?

> +	if (str_size > output.length) {
> +		dev_warn(&wdev->dev,
> +			 FW_WARN "WMI string size (%zu) exceeds WMI buffer size (%zu)\n",
> +			 str_size, output.length);
> +		return -EMSGSIZE;
> +	}
> +
> +	return wmi_string_to_utf8s(str, buf, length);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605181433.188847-1-W_Armin@gmx.de?part=5