Re: [PATCH v5 4/9] platform/x86: dell-wmi-base: 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:

Pre-existing issues:
- [High] Accessing `buffer[2]` when processing extended privacy events in `dell_wmi_process_key()` can lead to an out-of-bounds memory read.
- [High] Missing driver unregistration and resource cleanup on module load failure path in `dell_wmi_init()`.
--

commit 4cf1ebf24373946a98a04cc13f3df7ed1e23a5ad
Author: Armin Wolf <W_Armin@gmx.de>

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

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

> diff --git a/drivers/platform/x86/dell/dell-wmi-base.c b/drivers/platform/x86/dell/dell-wmi-base.c
> index 2a5804efd3ea8..1070df0658073 100644
> --- a/drivers/platform/x86/dell/dell-wmi-base.c
> +++ b/drivers/platform/x86/dell/dell-wmi-base.c

[ ... ]

> @@ -446,15 +448,15 @@ static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16
>  	} else if (type == 0x0011 && code == 0xe070 && remaining > 0) {
>  		dell_wmi_switch_event(&priv->tabletswitch_dev,
>  				      "Dell tablet mode switch",
> -				      SW_TABLET_MODE, !buffer[0]);
> +				      SW_TABLET_MODE, !le16_to_cpu(buffer[0]));
>  		return 1;
>  	} else if (type == 0x0012 && code == 0x000c && remaining > 0) {
>  		/* Eprivacy toggle, switch to "on" key entry for on events */
> -		if (buffer[0] == 2)
> +		if (le16_to_cpu(buffer[0]) == 2)
>  			key++;
>  		used = 1;
>  	} else if (type == 0x0012 && code == 0x000d && remaining > 0) {
> -		value = (buffer[2] == 2);
> +		value = (le16_to_cpu(buffer[2]) == 2);

[Severity: High]
This is a pre-existing issue, but does checking remaining > 0 guarantee that
buffer[2] is accessible here?

If the event payload leaves remaining as 1 or 2, could accessing buffer[2]
cause an out-of-bounds read?

>  		used = 1;
>  	}

[ ... ]

> @@ -825,10 +827,10 @@ static struct wmi_driver dell_wmi_driver = {
>  		.name = "dell-wmi",
>  	},
>  	.id_table = dell_wmi_id_table,
> -	.min_event_size = sizeof(u16),
> +	.min_event_size = sizeof(__le16),
>  	.probe = dell_wmi_probe,
>  	.remove = dell_wmi_remove,
> -	.notify = dell_wmi_notify,
> +	.notify_new = dell_wmi_notify,
>  };
>  
>  static int __init dell_wmi_init(void)

[Severity: High]
This is a pre-existing issue, but looking further down in dell_wmi_init():

	err = dell_privacy_register_driver();
	if (err)
		return err;

	return wmi_driver_register(&dell_wmi_driver);
}

If wmi_driver_register() fails, the function returns an error without
unregistering the privacy driver or reverting dell_wmi_events_set_enabled().

Since this is the module init function, couldn't this leave a dangling
dell_privacy_wmi_driver registered when the module is freed, potentially
causing a use-after-free later?

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