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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Incorrect sizeof argument used in strncmp for object_signature validation.
- [Low] The interface version validation implicitly accepts version 2 without documentation.
--

commit 5b70a5d7de7afeabc7feebda00fbadd39ea87afa
Author: Armin Wolf <W_Armin@gmx.de>

platform/x86: dell-descriptor: Use new buffer-based WMI API

This commit updates the dell-descriptor driver to use the new buffer-based
WMI API. This allows supporting ACPI firmware implementations that do not
use ACPI buffers for the descriptor.

> diff --git a/drivers/platform/x86/dell/dell-wmi-descriptor.c b/drivers/platform/x86/dell/dell-wmi-descriptor.c
> index c2a1802027190..d1e751d430210 100644
> --- a/drivers/platform/x86/dell/dell-wmi-descriptor.c
> +++ b/drivers/platform/x86/dell/dell-wmi-descriptor.c

[ ... ]

> @@ -88,77 +108,47 @@ bool dell_wmi_get_hotfix(u32 *hotfix)

[ ... ]

> -	if (strncmp(obj->string.pointer, "DELL WMI", 8) != 0) {
> -		dev_err(&wdev->dev, "Dell descriptor buffer has invalid signature (%8ph)\n",
> -			buffer);
> -		ret = -EINVAL;
> -		descriptor_valid = ret;
> -		goto out;
> +	if (strncmp(desc->object_signature, " WMI", sizeof(desc->vendor_signature))) {

[Severity: Low]
Is this using the correct size field? The strncmp compares object_signature
but passes sizeof(desc->vendor_signature) instead of
sizeof(desc->object_signature). While functionally benign right now since
both fields are exactly 4 bytes long, this looks like a copy-paste error
that could be fragile if the structure ever changes.

> +		dev_err(&wdev->dev, "Dell descriptor buffer has invalid object signature (%4ph)\n",
> +			desc->object_signature);
> +		descriptor_valid = -ENOMSG;
> +		return -ENOMSG;
>  	}
>  	descriptor_valid = 0;
>  
> -	if (buffer[2] != 0 && buffer[2] != 1)
> -		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%lu)\n",
> -			(unsigned long) buffer[2]);
> -
> -	priv = devm_kzalloc(&wdev->dev, sizeof(struct descriptor_priv),
> -	GFP_KERNEL);
> +	if (le32_to_cpu(desc->interface_version) > 2)

[Severity: Low]
Does this code intentionally change the supported version checks?
The previous code explicitly checked if the version was not 0 and not 1,
and warned otherwise. This new check implicitly accepts version 2 as a
valid, known version without warning.

Since the commit message doesn't mention adding support for a new interface
version, should this > 2 check be > 1 instead?

> +		dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
> +			 le32_to_cpu(desc->interface_version));

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