* [PATCH v6 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:41 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 2/9] platform/x86: dell-privacy: " Armin Wolf
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the descriptor.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
.../platform/x86/dell/dell-wmi-descriptor.c | 108 ++++++++----------
1 file changed, 48 insertions(+), 60 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-wmi-descriptor.c b/drivers/platform/x86/dell/dell-wmi-descriptor.c
index c2a180202719..179d5678b3ad 100644
--- a/drivers/platform/x86/dell/dell-wmi-descriptor.c
+++ b/drivers/platform/x86/dell/dell-wmi-descriptor.c
@@ -7,14 +7,35 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/acpi.h>
+#include <linux/cleanup.h>
+#include <linux/compiler_attributes.h>
#include <linux/list.h>
#include <linux/module.h>
+#include <linux/types.h>
#include <linux/wmi.h>
#include "dell-wmi-descriptor.h"
#define DELL_WMI_DESCRIPTOR_GUID "8D9DDCBC-A997-11DA-B012-B622A1EF5492"
+/*
+ * Descriptor buffer is 128 byte long and contains:
+ *
+ * Name Offset Length Value
+ * Vendor Signature 0 4 "DELL"
+ * Object Signature 4 4 " WMI"
+ * WMI Interface Version 8 4 <version>
+ * WMI buffer length 12 4 <length>
+ * WMI hotfix number 16 4 <hotfix>
+ */
+struct descriptor {
+ /* Both fields are NOT null-terminated */
+ char vendor_signature[4] __nonstring;
+ char object_signature[4] __nonstring;
+ __le32 interface_version;
+ __le32 buffer_length;
+ __le32 hotfix_number;
+} __packed;
+
struct descriptor_priv {
struct list_head list;
u32 interface_version;
@@ -88,77 +109,46 @@ bool dell_wmi_get_hotfix(u32 *hotfix)
}
EXPORT_SYMBOL_GPL(dell_wmi_get_hotfix);
-/*
- * Descriptor buffer is 128 byte long and contains:
- *
- * Name Offset Length Value
- * Vendor Signature 0 4 "DELL"
- * Object Signature 4 4 " WMI"
- * WMI Interface Version 8 4 <version>
- * WMI buffer length 12 4 <length>
- * WMI hotfix number 16 4 <hotfix>
- */
-static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
- const void *context)
+static int dell_wmi_descriptor_probe(struct wmi_device *wdev, const void *context)
{
- union acpi_object *obj = NULL;
struct descriptor_priv *priv;
- u32 *buffer;
+ struct wmi_buffer buffer;
int ret;
- obj = wmidev_block_query(wdev, 0);
- if (!obj) {
- dev_err(&wdev->dev, "failed to read Dell WMI descriptor\n");
- ret = -EIO;
- goto out;
- }
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- dev_err(&wdev->dev, "Dell descriptor has wrong type\n");
- ret = -EINVAL;
+ ret = wmidev_query_block(wdev, 0, &buffer, sizeof(struct descriptor));
+ if (ret < 0) {
descriptor_valid = ret;
- goto out;
+ return ret;
}
- /* Although it's not technically a failure, this would lead to
- * unexpected behavior
- */
- if (obj->buffer.length != 128) {
- dev_err(&wdev->dev,
- "Dell descriptor buffer has unexpected length (%d)\n",
- obj->buffer.length);
- ret = -EINVAL;
- descriptor_valid = ret;
- goto out;
- }
+ struct descriptor *desc __free(kfree) = buffer.data;
- buffer = (u32 *)obj->buffer.pointer;
+ if (strncmp(desc->vendor_signature, "DELL", sizeof(desc->vendor_signature))) {
+ dev_err(&wdev->dev, "Dell descriptor buffer has invalid vendor signature (%4ph)\n",
+ desc->vendor_signature);
+ descriptor_valid = -ENOMSG;
+ return -ENOMSG;
+ }
- 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->object_signature))) {
+ 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) > 1)
+ dev_warn(&wdev->dev, "Dell descriptor buffer has unknown version (%u)\n",
+ le32_to_cpu(desc->interface_version));
- if (!priv) {
- ret = -ENOMEM;
- goto out;
- }
+ priv = devm_kzalloc(&wdev->dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
- priv->interface_version = buffer[2];
- priv->size = buffer[3];
- priv->hotfix = buffer[4];
- ret = 0;
+ priv->interface_version = le32_to_cpu(desc->interface_version);
+ priv->size = le32_to_cpu(desc->buffer_length);
+ priv->hotfix = le32_to_cpu(desc->hotfix_number);
dev_set_drvdata(&wdev->dev, priv);
mutex_lock(&list_mutex);
list_add_tail(&priv->list, &wmi_list);
@@ -169,8 +159,6 @@ static int dell_wmi_descriptor_probe(struct wmi_device *wdev,
(unsigned long) priv->size,
(unsigned long) priv->hotfix);
-out:
- kfree(obj);
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 2/9] platform/x86: dell-privacy: Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
2026-06-10 20:34 ` [PATCH v6 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:45 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 3/9] platform/x86: dell-smbios-wmi: " Armin Wolf
` (6 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the device state.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/dell/dell-wmi-privacy.c | 77 +++++++++-----------
1 file changed, 34 insertions(+), 43 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-wmi-privacy.c b/drivers/platform/x86/dell/dell-wmi-privacy.c
index ed099a431ea4..f9d275b2f900 100644
--- a/drivers/platform/x86/dell/dell-wmi-privacy.c
+++ b/drivers/platform/x86/dell/dell-wmi-privacy.c
@@ -9,11 +9,14 @@
#include <linux/acpi.h>
#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/compiler_attributes.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
#include <linux/list.h>
#include <linux/leds.h>
#include <linux/module.h>
+#include <linux/types.h>
#include <linux/wmi.h>
#include "dell-wmi-privacy.h"
@@ -25,6 +28,26 @@
#define DELL_PRIVACY_CAMERA_EVENT 0x2
#define led_to_priv(c) container_of(c, struct privacy_wmi_data, cdev)
+/*
+ * Describes the Device State class exposed by BIOS which can be consumed by
+ * various applications interested in knowing the Privacy feature capabilities.
+ * class DeviceState
+ * {
+ * [key, read] string InstanceName;
+ * [read] boolean ReadOnly;
+ *
+ * [WmiDataId(1), read] uint32 DevicesSupported;
+ * 0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy Screen
+ *
+ * [WmiDataId(2), read] uint32 CurrentState;
+ * 0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
+ * };
+ */
+struct device_state {
+ __le32 devices_supported;
+ __le32 current_state;
+} __packed;
+
/*
* The wmi_list is used to store the privacy_priv struct with mutex protecting
*/
@@ -185,60 +208,28 @@ static struct attribute *privacy_attrs[] = {
};
ATTRIBUTE_GROUPS(privacy);
-/*
- * Describes the Device State class exposed by BIOS which can be consumed by
- * various applications interested in knowing the Privacy feature capabilities.
- * class DeviceState
- * {
- * [key, read] string InstanceName;
- * [read] boolean ReadOnly;
- *
- * [WmiDataId(1), read] uint32 DevicesSupported;
- * 0 - None; 0x1 - Microphone; 0x2 - Camera; 0x4 - ePrivacy Screen
- *
- * [WmiDataId(2), read] uint32 CurrentState;
- * 0 - Off; 1 - On; Bit0 - Microphone; Bit1 - Camera; Bit2 - ePrivacyScreen
- * };
- */
static int get_current_status(struct wmi_device *wdev)
{
struct privacy_wmi_data *priv = dev_get_drvdata(&wdev->dev);
- union acpi_object *obj_present;
- u32 *buffer;
- int ret = 0;
+ struct wmi_buffer buffer;
+ int ret;
if (!priv) {
dev_err(&wdev->dev, "dell privacy priv is NULL\n");
return -EINVAL;
}
+
/* check privacy support features and device states */
- obj_present = wmidev_block_query(wdev, 0);
- if (!obj_present) {
- dev_err(&wdev->dev, "failed to read Binary MOF\n");
- return -EIO;
- }
+ ret = wmidev_query_block(wdev, 0, &buffer, sizeof(struct device_state));
+ if (ret < 0)
+ return ret;
- if (obj_present->type != ACPI_TYPE_BUFFER) {
- dev_err(&wdev->dev, "Binary MOF is not a buffer!\n");
- ret = -EIO;
- goto obj_free;
- }
- /* Although it's not technically a failure, this would lead to
- * unexpected behavior
- */
- if (obj_present->buffer.length != 8) {
- dev_err(&wdev->dev, "Dell privacy buffer has unexpected length (%d)!\n",
- obj_present->buffer.length);
- ret = -EINVAL;
- goto obj_free;
- }
- buffer = (u32 *)obj_present->buffer.pointer;
- priv->features_present = buffer[0];
- priv->last_status = buffer[1];
+ struct device_state *state __free(kfree) = buffer.data;
-obj_free:
- kfree(obj_present);
- return ret;
+ priv->features_present = le32_to_cpu(state->devices_supported);
+ priv->last_status = le32_to_cpu(state->current_state);
+
+ return 0;
}
static int dell_privacy_micmute_led_set(struct led_classdev *led_cdev,
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 2/9] platform/x86: dell-privacy: Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 2/9] platform/x86: dell-privacy: " Armin Wolf
@ 2026-06-10 20:45 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:45 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] dell_privacy_has_mic_mute() accesses `priv->features_present` after releasing `list_mutex`, risking a Use-After-Free if the device is concurrently unbound.
--
commit c9150394b4cc5e06aab976ccc95fd348e2c66514
Author: Armin Wolf <W_Armin@gmx.de>
platform/x86: dell-privacy: 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 the device state.
> diff --git a/drivers/platform/x86/dell/dell-wmi-privacy.c b/drivers/platform/x86/dell/dell-wmi-privacy.c
> index ed099a431ea4d..f9d275b2f900c 100644
> --- a/drivers/platform/x86/dell/dell-wmi-privacy.c
> +++ b/drivers/platform/x86/dell/dell-wmi-privacy.c
[ ... ]
> @@ -185,60 +208,28 @@ static struct attribute *privacy_attrs[] = {
> };
> ATTRIBUTE_GROUPS(privacy);
>
[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the file
I noticed a potential use-after-free in dell_privacy_has_mic_mute().
dell_privacy_has_mic_mute() accesses priv->features_present after
releasing list_mutex:
bool dell_privacy_has_mic_mute(void)
{
struct privacy_wmi_data *priv;
mutex_lock(&list_mutex);
priv = list_first_entry_or_null(&wmi_list,
struct privacy_wmi_data,
list);
mutex_unlock(&list_mutex);
return priv && (priv->features_present & BIT(DELL_PRIVACY_TYPE_AUDIO));
}
If dell_privacy_wmi_remove() executes concurrently (e.g., via sysfs unbind)
and removes priv from the list, the devres core will automatically free the
devm_kzalloc-backed priv structure.
If the thread running dell_privacy_has_mic_mute() is preempted immediately
after unlocking list_mutex, will the subsequent dereference of
priv->features_present read freed memory?
> -/*
> - * Describes the Device State class exposed by BIOS which can be consumed by
> - * various applications interested in knowing the Privacy feature capabilities.
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610203453.816254-1-W_Armin@gmx.de?part=2
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 3/9] platform/x86: dell-smbios-wmi: Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
2026-06-10 20:34 ` [PATCH v6 1/9] platform/x86: dell-descriptor: Use new buffer-based WMI API Armin Wolf
2026-06-10 20:34 ` [PATCH v6 2/9] platform/x86: dell-privacy: " Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:47 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 4/9] platform/x86: dell-wmi-base: " Armin Wolf
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
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.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/dell/dell-smbios-wmi.c | 40 +++++++++------------
1 file changed, 17 insertions(+), 23 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-smbios-wmi.c b/drivers/platform/x86/dell/dell-smbios-wmi.c
index a7dca8c59d60..64d0871b706e 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)
static int run_smbios_call(struct wmi_device *wdev)
{
- struct acpi_buffer output = {ACPI_ALLOCATE_BUFFER, NULL};
- struct wmi_smbios_priv *priv;
- struct acpi_buffer input;
- union acpi_object *obj;
- acpi_status status;
-
- priv = dev_get_drvdata(&wdev->dev);
- input.length = priv->req_buf_size - sizeof(u64);
- input.pointer = &priv->buf->std;
+ struct wmi_smbios_priv *priv = dev_get_drvdata(&wdev->dev);
+ const struct wmi_buffer input = {
+ .length = priv->req_buf_size - sizeof(u64),
+ .data = &priv->buf->std,
+ };
+ struct wmi_buffer output;
+ int ret;
dev_dbg(&wdev->dev, "evaluating: %u/%u [%x,%x,%x,%x]\n",
priv->buf->std.cmd_class, priv->buf->std.cmd_select,
priv->buf->std.input[0], priv->buf->std.input[1],
priv->buf->std.input[2], priv->buf->std.input[3]);
- 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);
+ 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]);
- kfree(output.pointer);
return 0;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 3/9] platform/x86: dell-smbios-wmi: Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 3/9] platform/x86: dell-smbios-wmi: " Armin Wolf
@ 2026-06-10 20:47 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:47 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] If the ACPI WMI method successfully evaluates but returns no output object, `wmidev_invoke_method()` returns success and sets the output data to `ZERO_SIZE_PTR`, bypassing the `min_size` constraint. `run_smbios_call()` then unconditionally executes `memcpy` from `ZERO_SIZE_PTR`, leading to a kernel panic.
- [High] Use-After-Free (UAF) in the ioctl and read paths because the character device is unregistered using a devm action, allowing file operations to execute after the underlying device memory is freed.
--
commit ab9678fa1e5f61b73fc8e1705045d35055367552
Author: Armin Wolf <W_Armin@gmx.de>
platform/x86: dell-smbios-wmi: Use new buffer-based WMI API
This commit updates the dell-smbios-wmi driver to use the new buffer-based WMI
API. It transitions the driver to support ACPI firmware implementations that do
not rely on ACPI buffers for SMBIOS call results.
> 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)
>
> static int run_smbios_call(struct wmi_device *wdev)
> {
[ ... ]
> - 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 WMI method successfully evaluates but returns no output object?
If wmidev_invoke_method() in drivers/platform/wmi/core.c encounters a
missing ACPI object (!obj), it returns 0 and sets out->data to ZERO_SIZE_PTR:
status = wmidev_evaluate_method(wdev, instance, method_id, &ain, &aout);
...
obj = aout.pointer;
if (!obj) {
out->length = 0;
out->data = ZERO_SIZE_PTR;
return 0;
}
Since wmidev_invoke_method() returns success and bypasses the min_size check,
the memcpy() here would attempt to read input.length bytes from ZERO_SIZE_PTR,
leading to an invalid memory access.
[Severity: High]
This is a pre-existing issue, but does the character device unregistration
sequence allow for a use-after-free during device unbind?
The driver registers a character device via misc_register() and uses
devm_add_action_or_reset() to unregister it. It also allocates the priv
and priv->buf structures using devm_kzalloc() and devm_get_free_pages().
When the WMI device is removed (e.g., via sysfs unbind), the devm cleanup
actions call misc_deregister() and then free the priv memory. However,
misc_deregister() does not revoke existing open file descriptors.
If a userspace process holds the character device open while the device
is unbound, it can subsequently call ioctl() or read(). These operations
would dereference filp->private_data in dell_smbios_wmi_ioctl() (which
points to the freed priv struct) and access the freed priv->buf memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610203453.816254-1-W_Armin@gmx.de?part=3
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 4/9] platform/x86: dell-wmi-base: Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (2 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 3/9] platform/x86: dell-smbios-wmi: " Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:46 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 5/9] platform/x86: dell-ddv: " Armin Wolf
` (4 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI buffers for the event data.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/dell/dell-wmi-base.c | 60 +++++++++++------------
1 file changed, 28 insertions(+), 32 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-wmi-base.c b/drivers/platform/x86/dell/dell-wmi-base.c
index 2a5804efd3ea..997383ba1846 100644
--- a/drivers/platform/x86/dell/dell-wmi-base.c
+++ b/drivers/platform/x86/dell/dell-wmi-base.c
@@ -13,6 +13,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include <linux/compiler_attributes.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -414,7 +415,8 @@ static void dell_wmi_switch_event(struct input_dev **subdev,
input_sync(*subdev);
}
-static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16 *buffer, int remaining)
+static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, __le16 *buffer,
+ int remaining)
{
struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
const struct key_entry *key;
@@ -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);
used = 1;
}
@@ -463,24 +465,17 @@ static int dell_wmi_process_key(struct wmi_device *wdev, int type, int code, u16
return used;
}
-static void dell_wmi_notify(struct wmi_device *wdev,
- union acpi_object *obj)
+static void dell_wmi_notify(struct wmi_device *wdev, const struct wmi_buffer *buffer)
{
struct dell_wmi_priv *priv = dev_get_drvdata(&wdev->dev);
- u16 *buffer_entry, *buffer_end;
- acpi_size buffer_size;
+ __le16 *buffer_entry, *buffer_end;
+ size_t buffer_size;
int len, i;
- if (obj->type != ACPI_TYPE_BUFFER) {
- pr_warn("bad response type %x\n", obj->type);
- return;
- }
-
- pr_debug("Received WMI event (%*ph)\n",
- obj->buffer.length, obj->buffer.pointer);
+ pr_debug("Received WMI event (%*ph)\n", (int)buffer->length, buffer->data);
- buffer_entry = (u16 *)obj->buffer.pointer;
- buffer_size = obj->buffer.length/2;
+ buffer_entry = buffer->data;
+ buffer_size = buffer->length / 2;
buffer_end = buffer_entry + buffer_size;
/*
@@ -496,12 +491,12 @@ static void dell_wmi_notify(struct wmi_device *wdev,
* one event on devices with WMI interface version 0.
*/
if (priv->interface_version == 0 && buffer_entry < buffer_end)
- if (buffer_end > buffer_entry + buffer_entry[0] + 1)
- buffer_end = buffer_entry + buffer_entry[0] + 1;
+ if (buffer_end > buffer_entry + le16_to_cpu(buffer_entry[0]) + 1)
+ buffer_end = buffer_entry + le16_to_cpu(buffer_entry[0]) + 1;
while (buffer_entry < buffer_end) {
- len = buffer_entry[0];
+ len = le16_to_cpu(buffer_entry[0]);
if (len == 0)
break;
@@ -514,11 +509,11 @@ static void dell_wmi_notify(struct wmi_device *wdev,
pr_debug("Process buffer (%*ph)\n", len*2, buffer_entry);
- switch (buffer_entry[1]) {
+ switch (le16_to_cpu(buffer_entry[1])) {
case 0x0000: /* One key pressed or event occurred */
if (len > 2)
- dell_wmi_process_key(wdev, buffer_entry[1],
- buffer_entry[2],
+ dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+ le16_to_cpu(buffer_entry[2]),
buffer_entry + 3,
len - 3);
/* Extended data is currently ignored */
@@ -526,22 +521,23 @@ static void dell_wmi_notify(struct wmi_device *wdev,
case 0x0010: /* Sequence of keys pressed */
case 0x0011: /* Sequence of events occurred */
for (i = 2; i < len; ++i)
- i += dell_wmi_process_key(wdev, buffer_entry[1],
- buffer_entry[i],
+ i += dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+ le16_to_cpu(buffer_entry[i]),
buffer_entry + i,
len - i - 1);
break;
case 0x0012:
- if ((len > 4) && dell_privacy_process_event(buffer_entry[1], buffer_entry[3],
- buffer_entry[4]))
+ if ((len > 4) && dell_privacy_process_event(le16_to_cpu(buffer_entry[1]),
+ le16_to_cpu(buffer_entry[3]),
+ le16_to_cpu(buffer_entry[4])))
/* dell_privacy_process_event has handled the event */;
else if (len > 2)
- dell_wmi_process_key(wdev, buffer_entry[1], buffer_entry[2],
+ dell_wmi_process_key(wdev, le16_to_cpu(buffer_entry[1]),
+ le16_to_cpu(buffer_entry[2]),
buffer_entry + 3, len - 3);
break;
default: /* Unknown event */
- pr_info("Unknown WMI event type 0x%x\n",
- (int)buffer_entry[1]);
+ pr_info("Unknown WMI event type 0x%x\n", le16_to_cpu(buffer_entry[1]));
break;
}
@@ -825,10 +821,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)
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 4/9] platform/x86: dell-wmi-base: Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 4/9] platform/x86: dell-wmi-base: " Armin Wolf
@ 2026-06-10 20:46 ` sashiko-bot
0 siblings, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:46 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] An out-of-bounds read occurs in `dell_wmi_process_key()` when processing an Eprivacy toggle WMI event with a truncated payload.
- [Medium] The dynamically allocated `tabletswitch_dev` input device does not have its `dev.parent` field set before registration, violating input subsystem hierarchy guidelines.
--
commit 64182cf85689e767a05985e35c108182a367628c
Author: Armin Wolf <W_Armin@gmx.de>
platform/x86: dell-wmi-base: Use new buffer-based WMI API
This commit updates the driver to use the new buffer-based WMI API.
This also supports 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..997383ba18465 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]));
[Severity: Medium]
This isn't a bug introduced by this patch, but does dell_wmi_switch_event()
register the input device without assigning dev->dev.parent?
Looking at the dell_wmi_switch_event() definition:
if (!*subdev) {
struct input_dev *dev = input_allocate_device();
...
(dev)->name = devname;
(dev)->id.bustype = BUS_HOST;
if (input_register_device(dev)) {
Without explicitly setting dev->dev.parent before registration, the device
might appear under /sys/devices/virtual/input/ instead of correctly rooting
in the sysfs hierarchy.
> 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 this cause an out-of-bounds read
when the payload is truncated?
Since remaining > 0 only guarantees that buffer[0] is accessible, could
accessing buffer[2] read memory past the end of the validated WMI buffer
if the remaining event length is 1 or 2?
> used = 1;
> }
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610203453.816254-1-W_Armin@gmx.de?part=4
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 5/9] platform/x86: dell-ddv: Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (3 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 4/9] platform/x86: dell-wmi-base: " Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:47 ` sashiko-bot
2026-06-11 13:39 ` Ilpo Järvinen
2026-06-10 20:34 ` [PATCH v6 6/9] hwmon: (dell-smm) " Armin Wolf
` (3 subsequent siblings)
8 siblings, 2 replies; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
Use the new buffer-based WMI API to also support ACPI firmware
implementations that do not use ACPI intergers/strings/packages
for exchanging data.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/x86/dell/dell-wmi-ddv.c | 179 +++++++++++------------
1 file changed, 85 insertions(+), 94 deletions(-)
diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
index 62e3d060f038..736d9b1fdcfb 100644
--- a/drivers/platform/x86/dell/dell-wmi-ddv.c
+++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
@@ -7,8 +7,9 @@
#define pr_format(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/acpi.h>
#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/compiler_attributes.h>
#include <linux/debugfs.h>
#include <linux/device.h>
#include <linux/device/driver.h>
@@ -99,6 +100,11 @@ enum dell_ddv_method {
DELL_DDV_THERMAL_SENSOR_INFORMATION = 0x22,
};
+struct dell_wmi_buffer {
+ __le32 raw_size;
+ u8 raw_data[];
+} __packed;
+
struct fan_sensor_entry {
u8 type;
__le16 rpm;
@@ -126,7 +132,7 @@ struct dell_wmi_ddv_sensors {
bool active;
struct mutex lock; /* protect caching */
unsigned long timestamp;
- union acpi_object *obj;
+ struct dell_wmi_buffer *buffer;
u64 entries;
};
@@ -158,105 +164,96 @@ static const char * const fan_dock_labels[] = {
"Docking Chipset Fan",
};
-static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
- union acpi_object **result, acpi_object_type type)
+static int dell_wmi_ddv_query(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
+ struct wmi_buffer *output, size_t min_size)
{
- struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
- const struct acpi_buffer in = {
- .length = sizeof(arg),
- .pointer = &arg,
+ __le32 arg2 = cpu_to_le32(arg);
+ const struct wmi_buffer input = {
+ .length = sizeof(arg2),
+ .data = &arg2,
};
- union acpi_object *obj;
- acpi_status ret;
-
- ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
- if (ACPI_FAILURE(ret))
- return -EIO;
-
- obj = out.pointer;
- if (!obj)
- return -ENODATA;
-
- if (obj->type != type) {
- kfree(obj);
- return -ENOMSG;
- }
-
- *result = obj;
- return 0;
+ return wmidev_invoke_method(wdev, 0x0, method, &input, output, min_size);
}
static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
u32 arg, u32 *res)
{
- union acpi_object *obj;
+ 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(__le32));
if (ret < 0)
return ret;
- if (obj->integer.value <= U32_MAX)
- *res = (u32)obj->integer.value;
- else
- ret = -ERANGE;
+ __le32 *argr __free(kfree) = output.data;
- kfree(obj);
+ *res = le32_to_cpu(*argr);
- 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)) {
ret = -ENODATA;
goto err_free;
}
- if (buffer_size > obj->package.elements[1].buffer.length) {
+ buffer_size = struct_size(buffer, raw_data, le32_to_cpu(buffer->raw_size));
+ if (buffer_size > output.length) {
dev_warn(&wdev->dev,
- FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
- buffer_size, obj->package.elements[1].buffer.length);
+ FW_WARN "Dell WMI buffer size (%zu) exceeds WMI buffer size (%zu)\n",
+ buffer_size, output.length);
ret = -EMSGSIZE;
goto err_free;
}
- *result = obj;
+ *result = buffer;
return 0;
err_free:
- kfree(obj);
+ kfree(output.data);
return ret;
}
-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_buffer output;
+ size_t str_size;
+ int ret;
+
+ ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(struct wmi_string));
+ if (ret < 0)
+ return ret;
+
+ struct wmi_string *str __free(kfree) = output.data;
+
+ str_size = sizeof(*str) + le16_to_cpu(str->length);
+ 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);
}
/*
@@ -265,28 +262,26 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
struct dell_wmi_ddv_sensors *sensors, size_t entry_size)
{
+ struct dell_wmi_buffer *buffer;
u64 buffer_size, rem, entries;
- union acpi_object *obj;
- u8 *buffer;
int ret;
- if (sensors->obj) {
+ if (sensors->buffer) {
if (time_before(jiffies, sensors->timestamp + HZ))
return 0;
- kfree(sensors->obj);
- sensors->obj = NULL;
+ kfree(sensors->buffer);
+ sensors->buffer = NULL;
}
- ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
+ ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &buffer);
if (ret < 0)
return ret;
/* buffer format sanity check */
- buffer_size = obj->package.elements[0].integer.value;
- buffer = obj->package.elements[1].buffer.pointer;
+ buffer_size = le32_to_cpu(buffer->raw_size);
entries = div64_u64_rem(buffer_size, entry_size, &rem);
- if (rem != 1 || buffer[buffer_size - 1] != 0xff) {
+ if (rem != 1 || buffer->raw_data[buffer_size - 1] != 0xff) {
ret = -ENOMSG;
goto err_free;
}
@@ -296,14 +291,14 @@ static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_me
goto err_free;
}
- sensors->obj = obj;
+ sensors->buffer = buffer;
sensors->entries = entries;
sensors->timestamp = jiffies;
return 0;
err_free:
- kfree(obj);
+ kfree(buffer);
return ret;
}
@@ -328,7 +323,7 @@ static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 att
if (channel >= data->fans.entries)
return -ENXIO;
- entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
+ entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
switch (attr) {
case hwmon_fan_input:
*val = get_unaligned_le16(&entry[channel].rpm);
@@ -354,7 +349,7 @@ static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 at
if (channel >= data->temps.entries)
return -ENXIO;
- entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
+ entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
switch (attr) {
case hwmon_temp_input:
*val = entry[channel].now * 1000;
@@ -411,7 +406,7 @@ static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int chan
if (channel >= data->fans.entries)
return -ENXIO;
- entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
+ entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
type = entry[channel].type;
switch (type) {
case 0x00 ... 0x07:
@@ -442,7 +437,7 @@ static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int cha
if (channel >= data->temps.entries)
return -ENXIO;
- entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
+ entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
switch (entry[channel].type) {
case 0x00:
*str = "CPU";
@@ -553,8 +548,8 @@ static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sen
return;
mutex_lock(&sensors->lock);
- kfree(sensors->obj);
- sensors->obj = NULL;
+ kfree(sensors->buffer);
+ sensors->buffer = NULL;
mutex_unlock(&sensors->lock);
}
@@ -564,7 +559,7 @@ static void dell_wmi_ddv_hwmon_cache_destroy(void *data)
sensors->active = false;
mutex_destroy(&sensors->lock);
- kfree(sensors->obj);
+ kfree(sensors->buffer);
}
static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
@@ -750,7 +745,7 @@ static void dell_wmi_battery_invalidate(struct dell_wmi_ddv_data *data,
static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
- union acpi_object *obj;
+ ssize_t count;
u32 index;
int ret;
@@ -758,19 +753,19 @@ static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, cha
if (ret < 0)
return ret;
- ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
- if (ret < 0)
- return ret;
-
- if (obj->string.length != DELL_EPPID_LENGTH && obj->string.length != DELL_EPPID_EXT_LENGTH)
- dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%d)\n",
- obj->string.length);
+ count = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, buf,
+ PAGE_SIZE);
+ if (count < 0)
+ return count;
- ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
+ if (count != DELL_EPPID_LENGTH && count != DELL_EPPID_EXT_LENGTH)
+ dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%zd)\n", count);
- kfree(obj);
+ ret = sysfs_emit_at(buf, count, "\n");
+ if (ret < 0)
+ return ret;
- return ret;
+ return count + ret;
}
static int dell_wmi_ddv_get_health(struct dell_wmi_ddv_data *data, u32 index,
@@ -994,19 +989,15 @@ static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method m
{
struct device *dev = seq->private;
struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
- union acpi_object *obj;
- u64 size;
- u8 *buf;
+ struct dell_wmi_buffer *buffer;
int ret;
- ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
+ ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &buffer);
if (ret < 0)
return ret;
- size = obj->package.elements[0].integer.value;
- buf = obj->package.elements[1].buffer.pointer;
- ret = seq_write(seq, buf, size);
- kfree(obj);
+ ret = seq_write(seq, buffer->raw_data, le32_to_cpu(buffer->raw_size));
+ kfree(buffer);
return ret;
}
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 5/9] platform/x86: dell-ddv: Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 5/9] platform/x86: dell-ddv: " Armin Wolf
@ 2026-06-10 20:47 ` sashiko-bot
2026-06-11 13:39 ` Ilpo Järvinen
1 sibling, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:47 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Kernel panic due to dereferencing ZERO_SIZE_PTR when WMI methods return an empty result, bypassing length validation.
--
commit 1a33f1fa92cf5d49dd49bba56bde6f9f8debd47d
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..736d9b1fdcfbb 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;
> + 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(__le32));
> if (ret < 0)
> return ret;
>
> - if (obj->integer.value <= U32_MAX)
> - *res = (u32)obj->integer.value;
> - else
> - ret = -ERANGE;
> + __le32 *argr __free(kfree) = output.data;
>
> - kfree(obj);
> + *res = le32_to_cpu(*argr);
[Severity: High]
If the ACPI evaluation succeeds but returns no object, wmidev_invoke_method()
returns 0 and sets output.data to ZERO_SIZE_PTR.
The old code explicitly checked for this:
obj = out.pointer;
if (!obj)
return -ENODATA;
Does this code now dereference a ZERO_SIZE_PTR since we don't verify
that output.length > 0 before accessing *argr?
>
> - 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]
Does this also cause a ZERO_SIZE_PTR dereference if output.length is 0 and
output.data is ZERO_SIZE_PTR?
[ ... ]
> -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_buffer output;
> + size_t str_size;
> + int ret;
> +
> + ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(struct wmi_string));
> + if (ret < 0)
> + return ret;
> +
> + struct wmi_string *str __free(kfree) = output.data;
> +
> + str_size = sizeof(*str) + le16_to_cpu(str->length);
[Severity: High]
Similarly, does this code trigger a ZERO_SIZE_PTR dereference of str->length
if wmidev_invoke_method() returns 0 with no object?
> + 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/20260610203453.816254-1-W_Armin@gmx.de?part=5
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 5/9] platform/x86: dell-ddv: Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 5/9] platform/x86: dell-ddv: " Armin Wolf
2026-06-10 20:47 ` sashiko-bot
@ 2026-06-11 13:39 ` Ilpo Järvinen
1 sibling, 0 replies; 23+ messages in thread
From: Ilpo Järvinen @ 2026-06-11 13:39 UTC (permalink / raw)
To: Armin Wolf
Cc: Dell.Client.Kernel, pali, mjg59, soyer, Hans de Goede,
platform-driver-x86, LKML, linux, linux-hwmon, mario.limonciello
On Wed, 10 Jun 2026, Armin Wolf wrote:
> Use the new buffer-based WMI API to also support ACPI firmware
> implementations that do not use ACPI intergers/strings/packages
> for exchanging data.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> drivers/platform/x86/dell/dell-wmi-ddv.c | 179 +++++++++++------------
> 1 file changed, 85 insertions(+), 94 deletions(-)
>
> diff --git a/drivers/platform/x86/dell/dell-wmi-ddv.c b/drivers/platform/x86/dell/dell-wmi-ddv.c
> index 62e3d060f038..736d9b1fdcfb 100644
> --- a/drivers/platform/x86/dell/dell-wmi-ddv.c
> +++ b/drivers/platform/x86/dell/dell-wmi-ddv.c
> @@ -7,8 +7,9 @@
>
> #define pr_format(fmt) KBUILD_MODNAME ": " fmt
>
> -#include <linux/acpi.h>
> #include <linux/bitfield.h>
> +#include <linux/cleanup.h>
> +#include <linux/compiler_attributes.h>
> #include <linux/debugfs.h>
> #include <linux/device.h>
> #include <linux/device/driver.h>
> @@ -99,6 +100,11 @@ enum dell_ddv_method {
> DELL_DDV_THERMAL_SENSOR_INFORMATION = 0x22,
> };
>
> +struct dell_wmi_buffer {
> + __le32 raw_size;
> + u8 raw_data[];
> +} __packed;
> +
> struct fan_sensor_entry {
> u8 type;
> __le16 rpm;
> @@ -126,7 +132,7 @@ struct dell_wmi_ddv_sensors {
> bool active;
> struct mutex lock; /* protect caching */
> unsigned long timestamp;
> - union acpi_object *obj;
> + struct dell_wmi_buffer *buffer;
> u64 entries;
> };
>
> @@ -158,105 +164,96 @@ static const char * const fan_dock_labels[] = {
> "Docking Chipset Fan",
> };
>
> -static int dell_wmi_ddv_query_type(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
> - union acpi_object **result, acpi_object_type type)
> +static int dell_wmi_ddv_query(struct wmi_device *wdev, enum dell_ddv_method method, u32 arg,
> + struct wmi_buffer *output, size_t min_size)
> {
> - struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
> - const struct acpi_buffer in = {
> - .length = sizeof(arg),
> - .pointer = &arg,
> + __le32 arg2 = cpu_to_le32(arg);
> + const struct wmi_buffer input = {
> + .length = sizeof(arg2),
> + .data = &arg2,
> };
> - union acpi_object *obj;
> - acpi_status ret;
> -
> - ret = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
> - if (ACPI_FAILURE(ret))
> - return -EIO;
> -
> - obj = out.pointer;
> - if (!obj)
> - return -ENODATA;
> -
> - if (obj->type != type) {
> - kfree(obj);
> - return -ENOMSG;
> - }
> -
> - *result = obj;
>
> - return 0;
> + return wmidev_invoke_method(wdev, 0x0, method, &input, output, min_size);
> }
>
> static int dell_wmi_ddv_query_integer(struct wmi_device *wdev, enum dell_ddv_method method,
> u32 arg, u32 *res)
> {
> - union acpi_object *obj;
> + 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(__le32));
> if (ret < 0)
> return ret;
>
> - if (obj->integer.value <= U32_MAX)
> - *res = (u32)obj->integer.value;
> - else
> - ret = -ERANGE;
> + __le32 *argr __free(kfree) = output.data;
>
> - kfree(obj);
> + *res = le32_to_cpu(*argr);
>
> - 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)) {
> ret = -ENODATA;
>
> goto err_free;
> }
>
> - if (buffer_size > obj->package.elements[1].buffer.length) {
> + buffer_size = struct_size(buffer, raw_data, le32_to_cpu(buffer->raw_size));
> + if (buffer_size > output.length) {
> dev_warn(&wdev->dev,
> - FW_WARN "WMI buffer size (%llu) exceeds ACPI buffer size (%d)\n",
> - buffer_size, obj->package.elements[1].buffer.length);
> + FW_WARN "Dell WMI buffer size (%zu) exceeds WMI buffer size (%zu)\n",
> + buffer_size, output.length);
> ret = -EMSGSIZE;
>
> goto err_free;
> }
>
> - *result = obj;
> + *result = buffer;
This could use __free() and no_free_ptr() to simplify the code flow.
>
> return 0;
>
> err_free:
> - kfree(obj);
> + kfree(output.data);
>
> return ret;
> }
>
> -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_buffer output;
> + size_t str_size;
> + int ret;
> +
> + ret = dell_wmi_ddv_query(wdev, method, arg, &output, sizeof(struct wmi_string));
> + if (ret < 0)
> + return ret;
> +
> + struct wmi_string *str __free(kfree) = output.data;
> +
> + str_size = sizeof(*str) + le16_to_cpu(str->length);
> + 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);
> }
>
> /*
> @@ -265,28 +262,26 @@ static int dell_wmi_ddv_query_string(struct wmi_device *wdev, enum dell_ddv_meth
> static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_method method,
> struct dell_wmi_ddv_sensors *sensors, size_t entry_size)
> {
> + struct dell_wmi_buffer *buffer;
> u64 buffer_size, rem, entries;
> - union acpi_object *obj;
> - u8 *buffer;
> int ret;
>
> - if (sensors->obj) {
> + if (sensors->buffer) {
> if (time_before(jiffies, sensors->timestamp + HZ))
> return 0;
>
> - kfree(sensors->obj);
> - sensors->obj = NULL;
> + kfree(sensors->buffer);
> + sensors->buffer = NULL;
> }
>
> - ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &obj);
> + ret = dell_wmi_ddv_query_buffer(wdev, method, 0, &buffer);
> if (ret < 0)
> return ret;
>
> /* buffer format sanity check */
> - buffer_size = obj->package.elements[0].integer.value;
> - buffer = obj->package.elements[1].buffer.pointer;
> + buffer_size = le32_to_cpu(buffer->raw_size);
> entries = div64_u64_rem(buffer_size, entry_size, &rem);
> - if (rem != 1 || buffer[buffer_size - 1] != 0xff) {
> + if (rem != 1 || buffer->raw_data[buffer_size - 1] != 0xff) {
> ret = -ENOMSG;
> goto err_free;
> }
> @@ -296,14 +291,14 @@ static int dell_wmi_ddv_update_sensors(struct wmi_device *wdev, enum dell_ddv_me
> goto err_free;
> }
>
> - sensors->obj = obj;
> + sensors->buffer = buffer;
> sensors->entries = entries;
> sensors->timestamp = jiffies;
>
> return 0;
>
> err_free:
> - kfree(obj);
> + kfree(buffer);
>
> return ret;
> }
> @@ -328,7 +323,7 @@ static int dell_wmi_ddv_fan_read_channel(struct dell_wmi_ddv_data *data, u32 att
> if (channel >= data->fans.entries)
> return -ENXIO;
>
> - entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
> + entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
> switch (attr) {
> case hwmon_fan_input:
> *val = get_unaligned_le16(&entry[channel].rpm);
> @@ -354,7 +349,7 @@ static int dell_wmi_ddv_temp_read_channel(struct dell_wmi_ddv_data *data, u32 at
> if (channel >= data->temps.entries)
> return -ENXIO;
>
> - entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
> + entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
> switch (attr) {
> case hwmon_temp_input:
> *val = entry[channel].now * 1000;
> @@ -411,7 +406,7 @@ static int dell_wmi_ddv_fan_read_string(struct dell_wmi_ddv_data *data, int chan
> if (channel >= data->fans.entries)
> return -ENXIO;
>
> - entry = (struct fan_sensor_entry *)data->fans.obj->package.elements[1].buffer.pointer;
> + entry = (struct fan_sensor_entry *)data->fans.buffer->raw_data;
> type = entry[channel].type;
> switch (type) {
> case 0x00 ... 0x07:
> @@ -442,7 +437,7 @@ static int dell_wmi_ddv_temp_read_string(struct dell_wmi_ddv_data *data, int cha
> if (channel >= data->temps.entries)
> return -ENXIO;
>
> - entry = (struct thermal_sensor_entry *)data->temps.obj->package.elements[1].buffer.pointer;
> + entry = (struct thermal_sensor_entry *)data->temps.buffer->raw_data;
> switch (entry[channel].type) {
> case 0x00:
> *str = "CPU";
> @@ -553,8 +548,8 @@ static void dell_wmi_ddv_hwmon_cache_invalidate(struct dell_wmi_ddv_sensors *sen
> return;
>
> mutex_lock(&sensors->lock);
> - kfree(sensors->obj);
> - sensors->obj = NULL;
> + kfree(sensors->buffer);
> + sensors->buffer = NULL;
> mutex_unlock(&sensors->lock);
> }
>
> @@ -564,7 +559,7 @@ static void dell_wmi_ddv_hwmon_cache_destroy(void *data)
>
> sensors->active = false;
> mutex_destroy(&sensors->lock);
> - kfree(sensors->obj);
> + kfree(sensors->buffer);
> }
>
> static struct hwmon_channel_info *dell_wmi_ddv_channel_init(struct wmi_device *wdev,
> @@ -750,7 +745,7 @@ static void dell_wmi_battery_invalidate(struct dell_wmi_ddv_data *data,
> static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, char *buf)
> {
> struct dell_wmi_ddv_data *data = container_of(attr, struct dell_wmi_ddv_data, eppid_attr);
> - union acpi_object *obj;
> + ssize_t count;
> u32 index;
> int ret;
>
> @@ -758,19 +753,19 @@ static ssize_t eppid_show(struct device *dev, struct device_attribute *attr, cha
> if (ret < 0)
> return ret;
>
> - ret = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, &obj);
> - if (ret < 0)
> - return ret;
> -
> - if (obj->string.length != DELL_EPPID_LENGTH && obj->string.length != DELL_EPPID_EXT_LENGTH)
> - dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%d)\n",
> - obj->string.length);
> + count = dell_wmi_ddv_query_string(data->wdev, DELL_DDV_BATTERY_EPPID, index, buf,
> + PAGE_SIZE);
> + if (count < 0)
> + return count;
>
> - ret = sysfs_emit(buf, "%s\n", obj->string.pointer);
> + if (count != DELL_EPPID_LENGTH && count != DELL_EPPID_EXT_LENGTH)
> + dev_info_once(&data->wdev->dev, FW_INFO "Suspicious ePPID length (%zd)\n", count);
>
> - kfree(obj);
> + ret = sysfs_emit_at(buf, count, "\n");
What's going on with this change? I see it printing nothing but a newline
character anymore?
--
i.
> + if (ret < 0)
> + return ret;
>
> - return ret;
> + return count + ret;
> }
>
> static int dell_wmi_ddv_get_health(struct dell_wmi_ddv_data *data, u32 index,
> @@ -994,19 +989,15 @@ static int dell_wmi_ddv_buffer_read(struct seq_file *seq, enum dell_ddv_method m
> {
> struct device *dev = seq->private;
> struct dell_wmi_ddv_data *data = dev_get_drvdata(dev);
> - union acpi_object *obj;
> - u64 size;
> - u8 *buf;
> + struct dell_wmi_buffer *buffer;
> int ret;
>
> - ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &obj);
> + ret = dell_wmi_ddv_query_buffer(data->wdev, method, 0, &buffer);
> if (ret < 0)
> return ret;
>
> - size = obj->package.elements[0].integer.value;
> - buf = obj->package.elements[1].buffer.pointer;
> - ret = seq_write(seq, buf, size);
> - kfree(obj);
> + ret = seq_write(seq, buffer->raw_data, le32_to_cpu(buffer->raw_size));
> + kfree(buffer);
>
> return ret;
> }
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (4 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 5/9] platform/x86: dell-ddv: " Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:45 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 7/9] platform/wmi: Make wmi_bus_class const Armin Wolf
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
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 SMM call.
Acked-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/hwmon/dell-smm-hwmon.c | 49 ++++++++++++----------------------
1 file changed, 17 insertions(+), 32 deletions(-)
diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
index 038edffc1ac7..6ca2ea4bfe7d 100644
--- a/drivers/hwmon/dell-smm-hwmon.c
+++ b/drivers/hwmon/dell-smm-hwmon.c
@@ -12,8 +12,10 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#include <linux/acpi.h>
+#include <linux/align.h>
#include <linux/capability.h>
+#include <linux/cleanup.h>
+#include <linux/compiler_attributes.h>
#include <linux/cpu.h>
#include <linux/ctype.h>
#include <linux/delay.h>
@@ -36,10 +38,10 @@
#include <linux/thermal.h>
#include <linux/types.h>
#include <linux/uaccess.h>
+#include <linux/unaligned.h>
#include <linux/wmi.h>
#include <linux/i8k.h>
-#include <linux/unaligned.h>
#define I8K_SMM_FN_STATUS 0x0025
#define I8K_SMM_POWER_STATUS 0x0069
@@ -232,7 +234,7 @@ static const struct dell_smm_ops i8k_smm_ops = {
/*
* Call the System Management Mode BIOS over WMI.
*/
-static ssize_t wmi_parse_register(u8 *buffer, u32 length, unsigned int *reg)
+static ssize_t wmi_parse_register(u8 *buffer, size_t length, unsigned int *reg)
{
__le32 value;
u32 reg_size;
@@ -253,7 +255,7 @@ static ssize_t wmi_parse_register(u8 *buffer, u32 length, unsigned int *reg)
return reg_size + sizeof(reg_size);
}
-static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
+static int wmi_parse_response(u8 *buffer, size_t length, struct smm_regs *regs)
{
unsigned int *registers[] = {
®s->eax,
@@ -261,7 +263,7 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
®s->ecx,
®s->edx
};
- u32 offset = 0;
+ size_t offset = 0;
ssize_t ret;
int i;
@@ -273,19 +275,16 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
if (ret < 0)
return ret;
- offset += ret;
+ /* WMI aligns u32 integers on a 4 byte boundary */
+ offset = ALIGN(offset + ret, 4);
}
- if (offset != length)
- return -ENOMSG;
-
return 0;
}
static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
{
struct wmi_device *wdev = container_of(dev, struct wmi_device, dev);
- struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
u32 wmi_payload[] = {
sizeof(regs->eax),
regs->eax,
@@ -296,34 +295,20 @@ static int wmi_smm_call(struct device *dev, struct smm_regs *regs)
sizeof(regs->edx),
regs->edx
};
- const struct acpi_buffer in = {
+ const struct wmi_buffer in = {
.length = sizeof(wmi_payload),
- .pointer = &wmi_payload,
+ .data = &wmi_payload,
};
- union acpi_object *obj;
- acpi_status status;
+ struct wmi_buffer out;
int ret;
- status = wmidev_evaluate_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out);
- if (ACPI_FAILURE(status))
- return -EIO;
-
- obj = out.pointer;
- if (!obj)
- return -ENODATA;
-
- if (obj->type != ACPI_TYPE_BUFFER) {
- ret = -ENOMSG;
-
- goto err_free;
- }
-
- ret = wmi_parse_response(obj->buffer.pointer, obj->buffer.length, regs);
+ ret = wmidev_invoke_method(wdev, 0x0, DELL_SMM_LEGACY_EXECUTE, &in, &out, sizeof(__le32));
+ if (ret < 0)
+ return ret;
-err_free:
- kfree(obj);
+ u8 *response __free(kfree) = out.data;
- return ret;
+ return wmi_parse_response(response, out.length, regs);
}
static int dell_smm_call(const struct dell_smm_ops *ops, struct smm_regs *regs)
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
2026-06-10 20:34 ` [PATCH v6 6/9] hwmon: (dell-smm) " Armin Wolf
@ 2026-06-10 20:45 ` sashiko-bot
2026-06-10 21:29 ` Armin Wolf
0 siblings, 1 reply; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:45 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] wmi_parse_response incorrectly fails with -ENODATA when the firmware returns fewer than 4 registers, breaking support for valid short SMM responses.
--
commit 5defcd15874af970946561c0d13ad1b0850d56ca
Author: Armin Wolf <W_Armin@gmx.de>
hwmon: (dell-smm) Use new buffer-based WMI API
This commit updates the dell-smm-hwmon driver to use the new buffer-based WMI
API. This allows the driver to also support ACPI firmware implementations that
do not use ACPI buffers for returning the results of a SMM call.
> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
> index 038edffc1ac74..6ca2ea4bfe7d3 100644
> --- a/drivers/hwmon/dell-smm-hwmon.c
> +++ b/drivers/hwmon/dell-smm-hwmon.c
> @@ -261,22 +263,19 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
> ®s->ecx,
> ®s->edx
> };
> - u32 offset = 0;
> + size_t offset = 0;
> ssize_t ret;
> int i;
>
> for (i = 0; i < ARRAY_SIZE(registers); i++) {
> if (offset >= length)
> return -ENODATA;
>
> ret = wmi_parse_register(buffer + offset, length - offset, registers[i]);
> if (ret < 0)
> return ret;
>
> - offset += ret;
> + /* WMI aligns u32 integers on a 4 byte boundary */
> + offset = ALIGN(offset + ret, 4);
> }
>
> - if (offset != length)
> - return -ENOMSG;
> -
> return 0;
> }
[Severity: High]
Does wmi_parse_response() incorrectly fail with -ENODATA when the firmware
returns fewer than 4 registers, breaking support for valid short SMM responses?
Based on the changes, it appears the parsing loop previously allowed a short
response to be fully consumed (where offset == length), eventually verifying
it and successfully returning 0 while leaving any unprovided registers
unchanged.
With the updated code, if a BIOS legitimately returns a valid WMI response
containing 1, 2, or 3 registers (e.g., 8, 16, or 24 bytes), the loop parses
the provided registers and increments offset to equal length. On the
subsequent iteration, it evaluates offset >= length as true.
Instead of succeeding, will this now fail the entire SMM call with -ENODATA?
Could this functional regression break hardware monitoring (e.g., fan speed,
temperatures) on Dell systems whose BIOS provides short WMI SMM responses?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610203453.816254-1-W_Armin@gmx.de?part=6
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 6/9] hwmon: (dell-smm) Use new buffer-based WMI API
2026-06-10 20:45 ` sashiko-bot
@ 2026-06-10 21:29 ` Armin Wolf
0 siblings, 0 replies; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 21:29 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-hwmon
Am 10.06.26 um 22:45 schrieb sashiko-bot@kernel.org:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] wmi_parse_response incorrectly fails with -ENODATA when the firmware returns fewer than 4 registers, breaking support for valid short SMM responses.
> --
>
> commit 5defcd15874af970946561c0d13ad1b0850d56ca
> Author: Armin Wolf <W_Armin@gmx.de>
> hwmon: (dell-smm) Use new buffer-based WMI API
>
> This commit updates the dell-smm-hwmon driver to use the new buffer-based WMI
> API. This allows the driver to also support ACPI firmware implementations that
> do not use ACPI buffers for returning the results of a SMM call.
>
>> diff --git a/drivers/hwmon/dell-smm-hwmon.c b/drivers/hwmon/dell-smm-hwmon.c
>> index 038edffc1ac74..6ca2ea4bfe7d3 100644
>> --- a/drivers/hwmon/dell-smm-hwmon.c
>> +++ b/drivers/hwmon/dell-smm-hwmon.c
>> @@ -261,22 +263,19 @@ static int wmi_parse_response(u8 *buffer, u32 length, struct smm_regs *regs)
>> ®s->ecx,
>> ®s->edx
>> };
>> - u32 offset = 0;
>> + size_t offset = 0;
>> ssize_t ret;
>> int i;
>>
>> for (i = 0; i < ARRAY_SIZE(registers); i++) {
>> if (offset >= length)
>> return -ENODATA;
>>
>> ret = wmi_parse_register(buffer + offset, length - offset, registers[i]);
>> if (ret < 0)
>> return ret;
>>
>> - offset += ret;
>> + /* WMI aligns u32 integers on a 4 byte boundary */
>> + offset = ALIGN(offset + ret, 4);
>> }
>>
>> - if (offset != length)
>> - return -ENOMSG;
>> -
>> return 0;
>> }
> [Severity: High]
> Does wmi_parse_response() incorrectly fail with -ENODATA when the firmware
> returns fewer than 4 registers, breaking support for valid short SMM responses?
>
> Based on the changes, it appears the parsing loop previously allowed a short
> response to be fully consumed (where offset == length), eventually verifying
> it and successfully returning 0 while leaving any unprovided registers
> unchanged.
>
> With the updated code, if a BIOS legitimately returns a valid WMI response
> containing 1, 2, or 3 registers (e.g., 8, 16, or 24 bytes), the loop parses
> the provided registers and increments offset to equal length. On the
> subsequent iteration, it evaluates offset >= length as true.
>
> Instead of succeeding, will this now fail the entire SMM call with -ENODATA?
>
> Could this functional regression break hardware monitoring (e.g., fan speed,
> temperatures) on Dell systems whose BIOS provides short WMI SMM responses?
Short responses where never valid and where never accepted before.
Thanks,
Armin Wolf
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v6 7/9] platform/wmi: Make wmi_bus_class const
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (5 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 6/9] hwmon: (dell-smm) " Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:40 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 8/9] platform/wmi: Make sysfs attributes const Armin Wolf
2026-06-10 20:34 ` [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings Armin Wolf
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
The functions class_register()/_unregister() and device_create()
both support taking a const pointer to the class struct. Use this
to mark wmi_bus_class as const so that it can be placed into
read-only memory for better security.
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/wmi/core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index 5a2ffcbab6af..0782ebc33c00 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -1088,7 +1088,7 @@ static void wmi_dev_shutdown(struct device *dev)
}
}
-static struct class wmi_bus_class = {
+static const struct class wmi_bus_class = {
.name = "wmi_bus",
};
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 8/9] platform/wmi: Make sysfs attributes const
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (6 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 7/9] platform/wmi: Make wmi_bus_class const Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:43 ` sashiko-bot
2026-06-10 20:34 ` [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings Armin Wolf
8 siblings, 1 reply; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
The sysfs core supports const attributes. Use this to mark all
sysfs attributes as const so that they can be placed into read-only
memory for better security.
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
drivers/platform/wmi/core.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/drivers/platform/wmi/core.c b/drivers/platform/wmi/core.c
index 0782ebc33c00..529825dcfbfe 100644
--- a/drivers/platform/wmi/core.c
+++ b/drivers/platform/wmi/core.c
@@ -858,7 +858,8 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
return sysfs_emit(buf, "wmi:%pUL\n", &wblock->gblock.guid);
}
-static DEVICE_ATTR_RO(modalias);
+
+static const DEVICE_ATTR_RO(modalias);
static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -867,7 +868,8 @@ static ssize_t guid_show(struct device *dev, struct device_attribute *attr,
return sysfs_emit(buf, "%pUL\n", &wblock->gblock.guid);
}
-static DEVICE_ATTR_RO(guid);
+
+static const DEVICE_ATTR_RO(guid);
static ssize_t instance_count_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -876,7 +878,8 @@ static ssize_t instance_count_show(struct device *dev,
return sysfs_emit(buf, "%d\n", (int)wblock->gblock.instance_count);
}
-static DEVICE_ATTR_RO(instance_count);
+
+static const DEVICE_ATTR_RO(instance_count);
static ssize_t expensive_show(struct device *dev,
struct device_attribute *attr, char *buf)
@@ -886,9 +889,10 @@ static ssize_t expensive_show(struct device *dev,
return sysfs_emit(buf, "%d\n",
(wblock->gblock.flags & ACPI_WMI_EXPENSIVE) != 0);
}
-static DEVICE_ATTR_RO(expensive);
-static struct attribute *wmi_attrs[] = {
+static const DEVICE_ATTR_RO(expensive);
+
+static const struct attribute * const wmi_attrs[] = {
&dev_attr_modalias.attr,
&dev_attr_guid.attr,
&dev_attr_instance_count.attr,
@@ -904,9 +908,10 @@ static ssize_t notify_id_show(struct device *dev, struct device_attribute *attr,
return sysfs_emit(buf, "%02X\n", (unsigned int)wblock->gblock.notify_id);
}
-static DEVICE_ATTR_RO(notify_id);
-static struct attribute *wmi_event_attrs[] = {
+static const DEVICE_ATTR_RO(notify_id);
+
+static const struct attribute * const wmi_event_attrs[] = {
&dev_attr_notify_id.attr,
NULL
};
@@ -920,7 +925,8 @@ static ssize_t object_id_show(struct device *dev, struct device_attribute *attr,
return sysfs_emit(buf, "%c%c\n", wblock->gblock.object_id[0],
wblock->gblock.object_id[1]);
}
-static DEVICE_ATTR_RO(object_id);
+
+static const DEVICE_ATTR_RO(object_id);
static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
char *buf)
@@ -929,16 +935,17 @@ static ssize_t setable_show(struct device *dev, struct device_attribute *attr,
return sysfs_emit(buf, "%d\n", (int)wdev->setable);
}
-static DEVICE_ATTR_RO(setable);
-static struct attribute *wmi_data_attrs[] = {
+static const DEVICE_ATTR_RO(setable);
+
+static const struct attribute * const wmi_data_attrs[] = {
&dev_attr_object_id.attr,
&dev_attr_setable.attr,
NULL
};
ATTRIBUTE_GROUPS(wmi_data);
-static struct attribute *wmi_method_attrs[] = {
+static const struct attribute * const wmi_method_attrs[] = {
&dev_attr_object_id.attr,
NULL
};
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings
2026-06-10 20:34 [PATCH v6 0/9] Convert most Dell WMI drivers to use the new buffer-based API Armin Wolf
` (7 preceding siblings ...)
2026-06-10 20:34 ` [PATCH v6 8/9] platform/wmi: Make sysfs attributes const Armin Wolf
@ 2026-06-10 20:34 ` Armin Wolf
2026-06-10 20:50 ` sashiko-bot
2026-06-10 21:05 ` Pali Rohár
8 siblings, 2 replies; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 20:34 UTC (permalink / raw)
To: Dell.Client.Kernel, pali, mjg59
Cc: soyer, hansg, ilpo.jarvinen, platform-driver-x86, linux-kernel,
linux, linux-hwmon, mario.limonciello
Some WMI GUIDs found inside binary MOF files contain both
uppercase and lowercase characters. Blindly copying such
GUIDs will prevent the associated WMI driver from loading
automatically because the WMI GUID found inside WMI device ids
always contains uppercase characters.
Avoid this issue by always converting WMI GUID strings to
uppercase. Also verify that the WMI GUID string actually looks
like a valid GUID.
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
.../wmi/driver-development-guide.rst | 2 +-
scripts/mod/file2alias.c | 28 ++++++++++++++++++-
2 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
index 387f508d57ad..6290c448f5e7 100644
--- a/Documentation/wmi/driver-development-guide.rst
+++ b/Documentation/wmi/driver-development-guide.rst
@@ -54,7 +54,7 @@ to matching WMI devices using a struct wmi_device_id table:
::
static const struct wmi_device_id foo_id_table[] = {
- /* Only use uppercase letters! */
+ /* Using only uppercase letters is recommended */
{ "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
{ }
};
diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
index 4e99393a35f1..20e542a888c4 100644
--- a/scripts/mod/file2alias.c
+++ b/scripts/mod/file2alias.c
@@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, void *symval)
static void do_wmi_entry(struct module *mod, void *symval)
{
DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
+ char result[sizeof(*guid_string)];
+ int i;
if (strlen(*guid_string) != UUID_STRING_LEN) {
warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
@@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, void *symval)
return;
}
- module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", *guid_string);
+ for (i = 0; i < UUID_STRING_LEN; i++) {
+ char value = (*guid_string)[i];
+ bool valid = false;
+
+ if (i == 8 || i == 13 || i == 18 || i == 23) {
+ if (value == '-')
+ valid = true;
+ } else {
+ if (isxdigit(value))
+ valid = true;
+ }
+
+ if (!valid) {
+ warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
+ value, *guid_string, mod->name);
+ return;
+ }
+
+ /* Some GUIDs from BMOF definitions contain lowercase characters */
+ result[i] = toupper(value);
+ }
+
+ result[i] = '\0';
+
+ module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
}
/* Looks like: mhi:S */
--
2.39.5
^ permalink raw reply related [flat|nested] 23+ messages in thread* Re: [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings
2026-06-10 20:34 ` [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings Armin Wolf
@ 2026-06-10 20:50 ` sashiko-bot
2026-06-10 21:05 ` Pali Rohár
1 sibling, 0 replies; 23+ messages in thread
From: sashiko-bot @ 2026-06-10 20:50 UTC (permalink / raw)
To: Armin Wolf; +Cc: linux-hwmon
> Some WMI GUIDs found inside binary MOF files contain both
> uppercase and lowercase characters. Blindly copying such
> GUIDs will prevent the associated WMI driver from loading
> automatically because the WMI GUID found inside WMI device ids
> always contains uppercase characters.
>
> Avoid this issue by always converting WMI GUID strings to
> uppercase. Also verify that the WMI GUID string actually looks
> like a valid GUID.
>
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
Sashiko has reviewed this patch and found no issues. It looks great!
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260610203453.816254-1-W_Armin@gmx.de?part=9
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings
2026-06-10 20:34 ` [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings Armin Wolf
2026-06-10 20:50 ` sashiko-bot
@ 2026-06-10 21:05 ` Pali Rohár
2026-06-10 21:31 ` Armin Wolf
1 sibling, 1 reply; 23+ messages in thread
From: Pali Rohár @ 2026-06-10 21:05 UTC (permalink / raw)
To: Armin Wolf
Cc: Dell.Client.Kernel, mjg59, soyer, hansg, ilpo.jarvinen,
platform-driver-x86, linux-kernel, linux, linux-hwmon,
mario.limonciello
On Wednesday 10 June 2026 22:34:53 Armin Wolf wrote:
> Some WMI GUIDs found inside binary MOF files contain both
> uppercase and lowercase characters. Blindly copying such
> GUIDs will prevent the associated WMI driver from loading
> automatically because the WMI GUID found inside WMI device ids
> always contains uppercase characters.
>
> Avoid this issue by always converting WMI GUID strings to
> uppercase. Also verify that the WMI GUID string actually looks
> like a valid GUID.
Hello! Maybe different idea: Would not it be better to check in
do_wmi_entry() if all letters are really upper case?
Mixing lowercase and uppercase is a mess. And if somebody blindly copy
lowercase from MOF files to some kernel wmi driver, we can check for
this mistake in file2alias.c and throw an error.
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> .../wmi/driver-development-guide.rst | 2 +-
> scripts/mod/file2alias.c | 28 ++++++++++++++++++-
> 2 files changed, 28 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
> index 387f508d57ad..6290c448f5e7 100644
> --- a/Documentation/wmi/driver-development-guide.rst
> +++ b/Documentation/wmi/driver-development-guide.rst
> @@ -54,7 +54,7 @@ to matching WMI devices using a struct wmi_device_id table:
> ::
>
> static const struct wmi_device_id foo_id_table[] = {
> - /* Only use uppercase letters! */
> + /* Using only uppercase letters is recommended */
> { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
> { }
> };
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 4e99393a35f1..20e542a888c4 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, void *symval)
> static void do_wmi_entry(struct module *mod, void *symval)
> {
> DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
> + char result[sizeof(*guid_string)];
> + int i;
>
> if (strlen(*guid_string) != UUID_STRING_LEN) {
> warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
> @@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, void *symval)
> return;
> }
>
> - module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", *guid_string);
> + for (i = 0; i < UUID_STRING_LEN; i++) {
> + char value = (*guid_string)[i];
> + bool valid = false;
> +
> + if (i == 8 || i == 13 || i == 18 || i == 23) {
> + if (value == '-')
> + valid = true;
> + } else {
> + if (isxdigit(value))
> + valid = true;
> + }
> +
> + if (!valid) {
> + warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
> + value, *guid_string, mod->name);
> + return;
> + }
> +
> + /* Some GUIDs from BMOF definitions contain lowercase characters */
> + result[i] = toupper(value);
> + }
> +
> + result[i] = '\0';
> +
> + module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
> }
>
> /* Looks like: mhi:S */
> --
> 2.39.5
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH v6 9/9] modpost: Handle malformed WMI GUID strings
2026-06-10 21:05 ` Pali Rohár
@ 2026-06-10 21:31 ` Armin Wolf
0 siblings, 0 replies; 23+ messages in thread
From: Armin Wolf @ 2026-06-10 21:31 UTC (permalink / raw)
To: Pali Rohár
Cc: Dell.Client.Kernel, mjg59, soyer, hansg, ilpo.jarvinen,
platform-driver-x86, linux-kernel, linux, linux-hwmon,
mario.limonciello
Am 10.06.26 um 23:05 schrieb Pali Rohár:
> On Wednesday 10 June 2026 22:34:53 Armin Wolf wrote:
>> Some WMI GUIDs found inside binary MOF files contain both
>> uppercase and lowercase characters. Blindly copying such
>> GUIDs will prevent the associated WMI driver from loading
>> automatically because the WMI GUID found inside WMI device ids
>> always contains uppercase characters.
>>
>> Avoid this issue by always converting WMI GUID strings to
>> uppercase. Also verify that the WMI GUID string actually looks
>> like a valid GUID.
> Hello! Maybe different idea: Would not it be better to check in
> do_wmi_entry() if all letters are really upper case?
>
> Mixing lowercase and uppercase is a mess. And if somebody blindly copy
> lowercase from MOF files to some kernel wmi driver, we can check for
> this mistake in file2alias.c and throw an error.
guid_parse() handles mixed-case GUIDs just fine, so i prefer to accept them as well.
In the long term i am trying to move the whole WMI subsystem to guid_t or even class names
from the BMOF data.
Thanks,
Armin Wolf
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> .../wmi/driver-development-guide.rst | 2 +-
>> scripts/mod/file2alias.c | 28 ++++++++++++++++++-
>> 2 files changed, 28 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/wmi/driver-development-guide.rst b/Documentation/wmi/driver-development-guide.rst
>> index 387f508d57ad..6290c448f5e7 100644
>> --- a/Documentation/wmi/driver-development-guide.rst
>> +++ b/Documentation/wmi/driver-development-guide.rst
>> @@ -54,7 +54,7 @@ to matching WMI devices using a struct wmi_device_id table:
>> ::
>>
>> static const struct wmi_device_id foo_id_table[] = {
>> - /* Only use uppercase letters! */
>> + /* Using only uppercase letters is recommended */
>> { "936DA01F-9ABD-4D9D-80C7-02AF85C822A8", NULL },
>> { }
>> };
>> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
>> index 4e99393a35f1..20e542a888c4 100644
>> --- a/scripts/mod/file2alias.c
>> +++ b/scripts/mod/file2alias.c
>> @@ -1253,6 +1253,8 @@ static void do_tee_entry(struct module *mod, void *symval)
>> static void do_wmi_entry(struct module *mod, void *symval)
>> {
>> DEF_FIELD_ADDR(symval, wmi_device_id, guid_string);
>> + char result[sizeof(*guid_string)];
>> + int i;
>>
>> if (strlen(*guid_string) != UUID_STRING_LEN) {
>> warn("Invalid WMI device id 'wmi:%s' in '%s'\n",
>> @@ -1260,7 +1262,31 @@ static void do_wmi_entry(struct module *mod, void *symval)
>> return;
>> }
>>
>> - module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", *guid_string);
>> + for (i = 0; i < UUID_STRING_LEN; i++) {
>> + char value = (*guid_string)[i];
>> + bool valid = false;
>> +
>> + if (i == 8 || i == 13 || i == 18 || i == 23) {
>> + if (value == '-')
>> + valid = true;
>> + } else {
>> + if (isxdigit(value))
>> + valid = true;
>> + }
>> +
>> + if (!valid) {
>> + warn("Invalid character %c inside WMI GUID string '%s' in '%s'\n",
>> + value, *guid_string, mod->name);
>> + return;
>> + }
>> +
>> + /* Some GUIDs from BMOF definitions contain lowercase characters */
>> + result[i] = toupper(value);
>> + }
>> +
>> + result[i] = '\0';
>> +
>> + module_alias_printf(mod, false, WMI_MODULE_PREFIX "%s", result);
>> }
>>
>> /* Looks like: mhi:S */
>> --
>> 2.39.5
>>
^ permalink raw reply [flat|nested] 23+ messages in thread