| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <05f7b5b4-c692-4647-96d3-4935280e4097@gmx.de>
Date: Sat, 12 Apr 2025 02:30:27 +0200
From: Armin Wolf <W_Armin@....de>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, lkml@...heas.dev,
platform-driver-x86@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: msi-wmi-platform: Workaround a ACPI
firmware bug
Am 11.04.25 um 15:53 schrieb Ilpo Järvinen:
> On Thu, 10 Apr 2025, Armin Wolf wrote:
>
>> The ACPI byte code inside the ACPI control method responsible for
>> handling the WMI method calls uses a global buffer for constructing
>> the return value, yet the ACPI control method itself is not marked
>> as "Serialized".
>> This means that calling WMI methods on this WMI device is not
>> thread-safe, as concurrent WMI method calls will corrupt the global
>> buffer.
> Please avoid non-full lines in middle of a paragraph. Either make things
> truly own paragraphs or reflow the text in the paragraph.
>
>> Fix this by serializing the WMI method calls using a mutex.
>>
>> Fixes: 9c0beb6b29e7 ("platform/x86: wmi: Add MSI WMI Platform driver")
>> Tested-by: Antheas Kapenekakis <lkml@...heas.dev>
>> Signed-off-by: Armin Wolf <W_Armin@....de>
>> ---
>> .../wmi/devices/msi-wmi-platform.rst | 4 +
>> drivers/platform/x86/msi-wmi-platform.c | 99 ++++++++++++-------
>> 2 files changed, 67 insertions(+), 36 deletions(-)
>>
>> diff --git a/Documentation/wmi/devices/msi-wmi-platform.rst b/Documentation/wmi/devices/msi-wmi-platform.rst
>> index 31a136942892..73197b31926a 100644
>> --- a/Documentation/wmi/devices/msi-wmi-platform.rst
>> +++ b/Documentation/wmi/devices/msi-wmi-platform.rst
>> @@ -138,6 +138,10 @@ input data, the meaning of which depends on the subfeature being accessed.
>> The output buffer contains a single byte which signals success or failure (``0x00`` on failure)
>> and 31 bytes of output data, the meaning if which depends on the subfeature being accessed.
>>
>> +.. note::
>> + The ACPI control method responsible for handling the WMI method calls is not thread-safe.
>> + This is a firmware bug that needs to be handled inside the driver itself.
>> +
>> WMI method Get_EC()
>> -------------------
>>
>> diff --git a/drivers/platform/x86/msi-wmi-platform.c b/drivers/platform/x86/msi-wmi-platform.c
>> index 9b5c7f8c79b0..dc5e9878cb68 100644
>> --- a/drivers/platform/x86/msi-wmi-platform.c
>> +++ b/drivers/platform/x86/msi-wmi-platform.c
>> @@ -10,6 +10,7 @@
>> #include <linux/acpi.h>
>> #include <linux/bits.h>
>> #include <linux/bitfield.h>
>> +#include <linux/cleanup.h>
>> #include <linux/debugfs.h>
>> #include <linux/device.h>
>> #include <linux/device/driver.h>
>> @@ -17,6 +18,7 @@
>> #include <linux/hwmon.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> #include <linux/printk.h>
>> #include <linux/rwsem.h>
>> #include <linux/types.h>
>> @@ -76,8 +78,13 @@ enum msi_wmi_platform_method {
>> MSI_PLATFORM_GET_WMI = 0x1d,
>> };
>>
>> -struct msi_wmi_platform_debugfs_data {
>> +struct msi_wmi_platform_data {
>> struct wmi_device *wdev;
>> + struct mutex wmi_lock; /* Necessary when calling WMI methods */
>> +};
>> +
>> +struct msi_wmi_platform_debugfs_data {
>> + struct msi_wmi_platform_data *data;
>> enum msi_wmi_platform_method method;
>> struct rw_semaphore buffer_lock; /* Protects debugfs buffer */
>> size_t length;
>> @@ -132,8 +139,9 @@ static int msi_wmi_platform_parse_buffer(union acpi_object *obj, u8 *output, siz
>> return 0;
>> }
>>
>> -static int msi_wmi_platform_query(struct wmi_device *wdev, enum msi_wmi_platform_method method,
>> - u8 *input, size_t input_length, u8 *output, size_t output_length)
>> +static int msi_wmi_platform_query(struct msi_wmi_platform_data *data,
>> + enum msi_wmi_platform_method method, u8 *input,
>> + size_t input_length, u8 *output, size_t output_length)
>> {
>> struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
>> struct acpi_buffer in = {
>> @@ -147,9 +155,15 @@ static int msi_wmi_platform_query(struct wmi_device *wdev, enum msi_wmi_platform
>> if (!input_length || !output_length)
>> return -EINVAL;
>>
>> - status = wmidev_evaluate_method(wdev, 0x0, method, &in, &out);
>> - if (ACPI_FAILURE(status))
>> - return -EIO;
>> + /*
>> + * The ACPI control method responsible for handling the WMI method calls
>> + * is not thread-safe. Because of this we have to do the locking ourself.
>> + */
>> + scoped_guard(mutex, &data->wmi_lock) {
>> + status = wmidev_evaluate_method(data->wdev, 0x0, method, &in, &out);
>> + if (ACPI_FAILURE(status))
>> + return -EIO;
>> + }
>>
>> obj = out.pointer;
>> if (!obj)
>> @@ -170,22 +184,22 @@ static umode_t msi_wmi_platform_is_visible(const void *drvdata, enum hwmon_senso
>> static int msi_wmi_platform_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
>> int channel, long *val)
>> {
>> - struct wmi_device *wdev = dev_get_drvdata(dev);
>> + struct msi_wmi_platform_data *data = dev_get_drvdata(dev);
>> u8 input[32] = { 0 };
>> u8 output[32];
>> - u16 data;
>> + u16 value;
>> int ret;
>>
>> - ret = msi_wmi_platform_query(wdev, MSI_PLATFORM_GET_FAN, input, sizeof(input), output,
>> + ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_FAN, input, sizeof(input), output,
>> sizeof(output));
>> if (ret < 0)
>> return ret;
>>
>> - data = get_unaligned_be16(&output[channel * 2 + 1]);
>> - if (!data)
>> + value = get_unaligned_be16(&output[channel * 2 + 1]);
>> + if (!value)
>> *val = 0;
>> else
>> - *val = 480000 / data;
>> + *val = 480000 / value;
> Please put this variable rename into own patch before the actual fix.
Hi,
the variable rename is necessary because there would be a naming conflict with the struct msi_wmi_platform_data *data.
Since the rename is rather small i would prefer keeping this as a single patch to make it easier for the stable
team to backport.
Thanks,
Armin Wolf
> --
> i.
>
>> return 0;
>> }
>> @@ -231,7 +245,7 @@ static ssize_t msi_wmi_platform_write(struct file *fp, const char __user *input,
>> return ret;
>>
>> down_write(&data->buffer_lock);
>> - ret = msi_wmi_platform_query(data->wdev, data->method, payload, data->length, data->buffer,
>> + ret = msi_wmi_platform_query(data->data, data->method, payload, data->length, data->buffer,
>> data->length);
>> up_write(&data->buffer_lock);
>>
>> @@ -277,17 +291,17 @@ static void msi_wmi_platform_debugfs_remove(void *data)
>> debugfs_remove_recursive(dir);
>> }
>>
>> -static void msi_wmi_platform_debugfs_add(struct wmi_device *wdev, struct dentry *dir,
>> +static void msi_wmi_platform_debugfs_add(struct msi_wmi_platform_data *drvdata, struct dentry *dir,
>> const char *name, enum msi_wmi_platform_method method)
>> {
>> struct msi_wmi_platform_debugfs_data *data;
>> struct dentry *entry;
>>
>> - data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
>> + data = devm_kzalloc(&drvdata->wdev->dev, sizeof(*data), GFP_KERNEL);
>> if (!data)
>> return;
>>
>> - data->wdev = wdev;
>> + data->data = drvdata;
>> data->method = method;
>> init_rwsem(&data->buffer_lock);
>>
>> @@ -298,82 +312,82 @@ static void msi_wmi_platform_debugfs_add(struct wmi_device *wdev, struct dentry
>>
>> entry = debugfs_create_file(name, 0600, dir, data, &msi_wmi_platform_debugfs_fops);
>> if (IS_ERR(entry))
>> - devm_kfree(&wdev->dev, data);
>> + devm_kfree(&drvdata->wdev->dev, data);
>> }
>>
>> -static void msi_wmi_platform_debugfs_init(struct wmi_device *wdev)
>> +static void msi_wmi_platform_debugfs_init(struct msi_wmi_platform_data *data)
>> {
>> struct dentry *dir;
>> char dir_name[64];
>> int ret, method;
>>
>> - scnprintf(dir_name, ARRAY_SIZE(dir_name), "%s-%s", DRIVER_NAME, dev_name(&wdev->dev));
>> + scnprintf(dir_name, ARRAY_SIZE(dir_name), "%s-%s", DRIVER_NAME, dev_name(&data->wdev->dev));
>>
>> dir = debugfs_create_dir(dir_name, NULL);
>> if (IS_ERR(dir))
>> return;
>>
>> - ret = devm_add_action_or_reset(&wdev->dev, msi_wmi_platform_debugfs_remove, dir);
>> + ret = devm_add_action_or_reset(&data->wdev->dev, msi_wmi_platform_debugfs_remove, dir);
>> if (ret < 0)
>> return;
>>
>> for (method = MSI_PLATFORM_GET_PACKAGE; method <= MSI_PLATFORM_GET_WMI; method++)
>> - msi_wmi_platform_debugfs_add(wdev, dir, msi_wmi_platform_debugfs_names[method - 1],
>> + msi_wmi_platform_debugfs_add(data, dir, msi_wmi_platform_debugfs_names[method - 1],
>> method);
>> }
>>
>> -static int msi_wmi_platform_hwmon_init(struct wmi_device *wdev)
>> +static int msi_wmi_platform_hwmon_init(struct msi_wmi_platform_data *data)
>> {
>> struct device *hdev;
>>
>> - hdev = devm_hwmon_device_register_with_info(&wdev->dev, "msi_wmi_platform", wdev,
>> + hdev = devm_hwmon_device_register_with_info(&data->wdev->dev, "msi_wmi_platform", data,
>> &msi_wmi_platform_chip_info, NULL);
>>
>> return PTR_ERR_OR_ZERO(hdev);
>> }
>>
>> -static int msi_wmi_platform_ec_init(struct wmi_device *wdev)
>> +static int msi_wmi_platform_ec_init(struct msi_wmi_platform_data *data)
>> {
>> u8 input[32] = { 0 };
>> u8 output[32];
>> u8 flags;
>> int ret;
>>
>> - ret = msi_wmi_platform_query(wdev, MSI_PLATFORM_GET_EC, input, sizeof(input), output,
>> + ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_EC, input, sizeof(input), output,
>> sizeof(output));
>> if (ret < 0)
>> return ret;
>>
>> flags = output[MSI_PLATFORM_EC_FLAGS_OFFSET];
>>
>> - dev_dbg(&wdev->dev, "EC RAM version %lu.%lu\n",
>> + dev_dbg(&data->wdev->dev, "EC RAM version %lu.%lu\n",
>> FIELD_GET(MSI_PLATFORM_EC_MAJOR_MASK, flags),
>> FIELD_GET(MSI_PLATFORM_EC_MINOR_MASK, flags));
>> - dev_dbg(&wdev->dev, "EC firmware version %.28s\n",
>> + dev_dbg(&data->wdev->dev, "EC firmware version %.28s\n",
>> &output[MSI_PLATFORM_EC_VERSION_OFFSET]);
>>
>> if (!(flags & MSI_PLATFORM_EC_IS_TIGERLAKE)) {
>> if (!force)
>> return -ENODEV;
>>
>> - dev_warn(&wdev->dev, "Loading on a non-Tigerlake platform\n");
>> + dev_warn(&data->wdev->dev, "Loading on a non-Tigerlake platform\n");
>> }
>>
>> return 0;
>> }
>>
>> -static int msi_wmi_platform_init(struct wmi_device *wdev)
>> +static int msi_wmi_platform_init(struct msi_wmi_platform_data *data)
>> {
>> u8 input[32] = { 0 };
>> u8 output[32];
>> int ret;
>>
>> - ret = msi_wmi_platform_query(wdev, MSI_PLATFORM_GET_WMI, input, sizeof(input), output,
>> + ret = msi_wmi_platform_query(data, MSI_PLATFORM_GET_WMI, input, sizeof(input), output,
>> sizeof(output));
>> if (ret < 0)
>> return ret;
>>
>> - dev_dbg(&wdev->dev, "WMI interface version %u.%u\n",
>> + dev_dbg(&data->wdev->dev, "WMI interface version %u.%u\n",
>> output[MSI_PLATFORM_WMI_MAJOR_OFFSET],
>> output[MSI_PLATFORM_WMI_MINOR_OFFSET]);
>>
>> @@ -381,7 +395,8 @@ static int msi_wmi_platform_init(struct wmi_device *wdev)
>> if (!force)
>> return -ENODEV;
>>
>> - dev_warn(&wdev->dev, "Loading despite unsupported WMI interface version (%u.%u)\n",
>> + dev_warn(&data->wdev->dev,
>> + "Loading despite unsupported WMI interface version (%u.%u)\n",
>> output[MSI_PLATFORM_WMI_MAJOR_OFFSET],
>> output[MSI_PLATFORM_WMI_MINOR_OFFSET]);
>> }
>> @@ -391,19 +406,31 @@ static int msi_wmi_platform_init(struct wmi_device *wdev)
>>
>> static int msi_wmi_platform_probe(struct wmi_device *wdev, const void *context)
>> {
>> + struct msi_wmi_platform_data *data;
>> int ret;
>>
>> - ret = msi_wmi_platform_init(wdev);
>> + data = devm_kzalloc(&wdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (!data)
>> + return -ENOMEM;
>> +
>> + data->wdev = wdev;
>> + dev_set_drvdata(&wdev->dev, data);
>> +
>> + ret = devm_mutex_init(&wdev->dev, &data->wmi_lock);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = msi_wmi_platform_init(data);
>> if (ret < 0)
>> return ret;
>>
>> - ret = msi_wmi_platform_ec_init(wdev);
>> + ret = msi_wmi_platform_ec_init(data);
>> if (ret < 0)
>> return ret;
>>
>> - msi_wmi_platform_debugfs_init(wdev);
>> + msi_wmi_platform_debugfs_init(data);
>>
>> - return msi_wmi_platform_hwmon_init(wdev);
>> + return msi_wmi_platform_hwmon_init(data);
>> }
>>
>> static const struct wmi_device_id msi_wmi_platform_id_table[] = {
>> --
>> 2.39.5
>>
Powered by blists - more mailing lists