Re: [PATCH v4 3/9] platform/x86: dell-smbios-wmi: 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] If the ACPI WMI method succeeds but returns no data (an empty object), `wmidev_invoke_method()` sets `output.data` to `ZERO_SIZE_PTR` and returns 0 (success), ignoring `min_size`. `run_smbios_call()` then unconditionally calls `memcpy` with a large `input.length`, dereferencing `ZERO_SIZE_PTR` and causing a kernel panic.
- [High] Use-after-free on character device ioctl/read when the WMI device is unbound while file descriptors remain open.
--

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

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

Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for returning the
results of a SMBIOS call.

> diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
> index a7dca8c59d602..64d0871b706e5 100644
> --- a/drivers/platform/x86/dell/dell-smbios-wmi.c
> +++ b/drivers/platform/x86/dell/dell-smbios-wmi.c
> @@ -50,38 +50,32 @@ static inline struct wmi_smbios_priv *get_first_smbios_priv(void)

[ ... ]

> -	status = wmidev_evaluate_method(wdev, 0, 1, &input, &output);
> -	if (ACPI_FAILURE(status))
> -		return -EIO;
> -	obj = (union acpi_object *)output.pointer;
> -	if (obj->type != ACPI_TYPE_BUFFER) {
> -		dev_dbg(&wdev->dev, "received type: %d\n", obj->type);
> -		if (obj->type == ACPI_TYPE_INTEGER)
> -			dev_dbg(&wdev->dev, "SMBIOS call failed: %llu\n",
> -				obj->integer.value);
> -		kfree(output.pointer);
> -		return -EIO;
> -	}
> -	memcpy(input.pointer, obj->buffer.pointer, obj->buffer.length);
> +	/*
> +	 * The output buffer returned by the WMI method should have at least the size
> +	 * of the input buffer.
> +	 */
> +	ret = wmidev_invoke_method(wdev, 0, 1, &input, &output, input.length);
> +	if (ret < 0)
> +		return ret;
> +
> +	memcpy(input.data, output.data, input.length);

[Severity: High]
This is a pre-existing issue, but can this memcpy() cause a kernel panic?

If the ACPI WMI method evaluates successfully but returns no data (an empty
object), wmidev_invoke_method() in drivers/platform/wmi/core.c sets
out->data to ZERO_SIZE_PTR and returns 0:

	obj = aout.pointer;
	if (!obj) {
		out->length = 0;
		out->data = ZERO_SIZE_PTR;

		return 0;
	}

Since wmidev_invoke_method() returns 0 (success) and bypasses the min_size
check, the check for ret < 0 here passes. The memcpy() then unconditionally
dereferences ZERO_SIZE_PTR (0x10) for output.data with input.length, which
will trigger a fatal page fault.

> +	kfree(output.data);
>  	dev_dbg(&wdev->dev, "result: [%08x,%08x,%08x,%08x]\n",
>  		priv->buf->std.output[0], priv->buf->std.output[1],
>  		priv->buf->std.output[2], priv->buf->std.output[3]);

[ ... ]

[Severity: High]
This is a pre-existing issue, but could there be a use-after-free if the WMI
device is unbound while file descriptors remain open?

In dell_smbios_wmi_probe(), the driver uses devm_kzalloc() and
devm_get_free_pages() to allocate priv and priv->buf:

	priv = devm_kzalloc(&wdev->dev, sizeof(struct wmi_smbios_priv),
			    GFP_KERNEL);
	...
	priv->buf = (void *)devm_get_free_pages(&wdev->dev, GFP_KERNEL, count);

It registers a misc character device and ties its deregistration to devres.
If a userspace process has the device open when the WMI device is unbound,
devres will execute misc_deregister() and free the memory.

However, misc_deregister() does not revoke existing open file descriptors.
A subsequent ioctl() or read() on the open FD will access the freed priv pointer
and its buf in dell_smbios_wmi_ioctl():

	struct wmi_smbios_priv *priv = filp->private_data;
	...
	ret = dell_smbios_wmi_do_ioctl(priv, input);

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