Re: [RFC v3] hwmon: (hp-wmi-sensors) Fix failure to load on EliteDesk 800 G6

From: Guenter Roeck
Date: Thu Nov 30 2023 - 23:12:07 EST

Next message: Guenter Roeck: "Re: [PATCH v1 2/3] hwmon: tmp513: Simplify with dev_err_probe()"
Previous message: Like Xu: "Re: [PATCH] KVM: x86/pmu: Prevent any host user from enabling PEBS for profiling guest"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Nov 22, 2023 at 09:49:19PM -0800, James Seo wrote:
> The EliteDesk 800 G6 stores a raw WMI string within the ACPI object in its
> BIOS corresponding to one instance of HPBIOS_PlatformEvents.Name. This is
> evidently a valid way of representing a WMI data item as far as the
> Microsoft ACPI-WMI mapper is concerned, but is preventing the driver from
> loading.
>
> This seems quite rare, but add support for such strings. Treating this as a
> quirk pretty much means adding that support anyway.
>
> Also clean up an oversight in update_numeric_sensor_from_wobj() in which
> the result of hp_wmi_strdup() was being used without error checking.
>
> Reported-by: Lukasz Stelmach <l.stelmach@xxxxxxxxxxx>
> Closes: https://lore.kernel.org/linux-hwmon/7850a0bd-60e7-88f8-1d6c-0bb0e3234fdc@xxxxxxxxxxxx/
> Tested-by: Lukasz Stelmach <l.stelmach@xxxxxxxxxxx>
> Signed-off-by: James Seo <james@xxxxxxxxxx>

Applied.

Thanks,
Guenter

> ---
>
> Changes v2->v3:
> * Fix checkpatch warnings
> - Line break commit message differently (no non-whitespace changes)
> - Use "unsigned int" instead of "unsigned"
> * Add Tested-by: from Lukasz Stelmach
>
> Changes v1->v2:
> * Remove DMI-based workaround logic
> * Add full support for raw WMI strings
> - Improve UTF-16 validation and conversion for the general case
> - Support such strings if they occur in HPBIOS_BIOSEvent objects
> * Only use the result of hp_wmi_strdup() in update_numeric_sensor_from_wobj()
> if the call succeeded
>
> History:
> v2: https://lore.kernel.org/linux-hwmon/20231105192054.24833-1-james@xxxxxxxxxx/
> v1: https://lore.kernel.org/linux-hwmon/20231103181931.677796-1-james@xxxxxxxxxx/
>
> ---
> drivers/hwmon/hp-wmi-sensors.c | 127 ++++++++++++++++++++++++++++-----
> 1 file changed, 111 insertions(+), 16 deletions(-)
>
>
> base-commit: c03cd01890c5e41138cc5709e37859bc917aab5d
>
> diff --git a/drivers/hwmon/hp-wmi-sensors.c b/drivers/hwmon/hp-wmi-sensors.c
> index 17ae62f88bbf..b5325d0e72b9 100644
> --- a/drivers/hwmon/hp-wmi-sensors.c
> +++ b/drivers/hwmon/hp-wmi-sensors.c
> @@ -17,6 +17,8 @@
> * Available: https://github.com/linuxhw/ACPI
> * [4] P. Rohár, "bmfdec - Decompile binary MOF file (BMF) from WMI buffer",
> * 2017. [Online]. Available: https://github.com/pali/bmfdec
> + * [5] Microsoft Corporation, "Driver-Defined WMI Data Items", 2017. [Online].
> + * Available: https://learn.microsoft.com/en-us/windows-hardware/drivers/kernel/driver-defined-wmi-data-items
> */
>
> #include <linux/acpi.h>
> @@ -24,6 +26,7 @@
> #include <linux/hwmon.h>
> #include <linux/jiffies.h>
> #include <linux/mutex.h>
> +#include <linux/nls.h>
> #include <linux/units.h>
> #include <linux/wmi.h>
>
> @@ -395,6 +398,50 @@ struct hp_wmi_sensors {
> struct mutex lock; /* Lock polling WMI and driver state changes. */
> };
>
> +static bool is_raw_wmi_string(const u8 *pointer, u32 length)
> +{
> + const u16 *ptr;
> + u16 len;
> +
> + /* WMI strings are length-prefixed UTF-16 [5]. */
> + if (length <= sizeof(*ptr))
> + return false;
> +
> + length -= sizeof(*ptr);
> + ptr = (const u16 *)pointer;
> + len = *ptr;
> +
> + return len <= length && !(len & 1);
> +}
> +
> +static char *convert_raw_wmi_string(const u8 *buf)
> +{
> + const wchar_t *src;
> + unsigned int cps;
> + unsigned int len;
> + char *dst;
> + int i;
> +
> + src = (const wchar_t *)buf;
> +
> + /* Count UTF-16 code points. Exclude trailing null padding. */
> + cps = *src / sizeof(*src);
> + while (cps && !src[cps])
> + cps--;
> +
> + /* Each code point becomes up to 3 UTF-8 characters. */
> + len = min(cps * 3, HP_WMI_MAX_STR_SIZE - 1);
> +
> + dst = kmalloc((len + 1) * sizeof(*dst), GFP_KERNEL);
> + if (!dst)
> + return NULL;
> +
> + i = utf16s_to_utf8s(++src, cps, UTF16_LITTLE_ENDIAN, dst, len);
> + dst[i] = '\0';
> +
> + return dst;
> +}
> +
> /* hp_wmi_strdup - devm_kstrdup, but length-limited */
> static char *hp_wmi_strdup(struct device *dev, const char *src)
> {
> @@ -412,6 +459,23 @@ static char *hp_wmi_strdup(struct device *dev, const char *src)
> return dst;
> }
>
> +/* hp_wmi_wstrdup - hp_wmi_strdup, but for a raw WMI string */
> +static char *hp_wmi_wstrdup(struct device *dev, const u8 *buf)
> +{
> + char *src;
> + char *dst;
> +
> + src = convert_raw_wmi_string(buf);
> + if (!src)
> + return NULL;
> +
> + dst = hp_wmi_strdup(dev, strim(src)); /* Note: Copy is trimmed. */
> +
> + kfree(src);
> +
> + return dst;
> +}
> +
> /*
> * hp_wmi_get_wobj - poll WMI for a WMI object instance
> * @guid: WMI object GUID
> @@ -462,8 +526,14 @@ static int check_wobj(const union acpi_object *wobj,
> for (prop = 0; prop <= last_prop; prop++) {
> type = elements[prop].type;
> valid_type = property_map[prop];
> - if (type != valid_type)
> + if (type != valid_type) {
> + if (type == ACPI_TYPE_BUFFER &&
> + valid_type == ACPI_TYPE_STRING &&
> + is_raw_wmi_string(elements[prop].buffer.pointer,
> + elements[prop].buffer.length))
> + continue;
> return -EINVAL;
> + }
> }
>
> return 0;
> @@ -480,7 +550,9 @@ static int extract_acpi_value(struct device *dev,
> break;
>
> case ACPI_TYPE_STRING:
> - *out_string = hp_wmi_strdup(dev, strim(element->string.pointer));
> + *out_string = element->type == ACPI_TYPE_BUFFER ?
> + hp_wmi_wstrdup(dev, element->buffer.pointer) :
> + hp_wmi_strdup(dev, strim(element->string.pointer));
> if (!*out_string)
> return -ENOMEM;
> break;
> @@ -861,7 +933,9 @@ update_numeric_sensor_from_wobj(struct device *dev,
> {
> const union acpi_object *elements;
> const union acpi_object *element;
> - const char *string;
> + const char *new_string;
> + char *trimmed;
> + char *string;
> bool is_new;
> int offset;
> u8 size;
> @@ -885,11 +959,21 @@ update_numeric_sensor_from_wobj(struct device *dev,
> offset = is_new ? size - 1 : -2;
>
> element = &elements[HP_WMI_PROPERTY_CURRENT_STATE + offset];
> - string = strim(element->string.pointer);
> -
> - if (strcmp(string, nsensor->current_state)) {
> - devm_kfree(dev, nsensor->current_state);
> - nsensor->current_state = hp_wmi_strdup(dev, string);
> + string = element->type == ACPI_TYPE_BUFFER ?
> + convert_raw_wmi_string(element->buffer.pointer) :
> + element->string.pointer;
> +
> + if (string) {
> + trimmed = strim(string);
> + if (strcmp(trimmed, nsensor->current_state)) {
> + new_string = hp_wmi_strdup(dev, trimmed);
> + if (new_string) {
> + devm_kfree(dev, nsensor->current_state);
> + nsensor->current_state = new_string;
> + }
> + }
> + if (element->type == ACPI_TYPE_BUFFER)
> + kfree(string);
> }
>
> /* Old variant: -2 (not -1) because it lacks the Size property. */
> @@ -996,11 +1080,15 @@ static int check_event_wobj(const union acpi_object *wobj)
> HP_WMI_EVENT_PROPERTY_STATUS);
> }
>
> -static int populate_event_from_wobj(struct hp_wmi_event *event,
> +static int populate_event_from_wobj(struct device *dev,
> + struct hp_wmi_event *event,
> union acpi_object *wobj)
> {
> int prop = HP_WMI_EVENT_PROPERTY_NAME;
> union acpi_object *element;
> + acpi_object_type type;
> + char *string;
> + u32 value;
> int err;
>
> err = check_event_wobj(wobj);
> @@ -1009,20 +1097,24 @@ static int populate_event_from_wobj(struct hp_wmi_event *event,
>
> element = wobj->package.elements;
>
> - /* Extracted strings are NOT device-managed copies. */
> -
> for (; prop <= HP_WMI_EVENT_PROPERTY_CATEGORY; prop++, element++) {
> + type = hp_wmi_event_property_map[prop];
> +
> + err = extract_acpi_value(dev, element, type, &value, &string);
> + if (err)
> + return err;
> +
> switch (prop) {
> case HP_WMI_EVENT_PROPERTY_NAME:
> - event->name = strim(element->string.pointer);
> + event->name = string;
> break;
>
> case HP_WMI_EVENT_PROPERTY_DESCRIPTION:
> - event->description = strim(element->string.pointer);
> + event->description = string;
> break;
>
> case HP_WMI_EVENT_PROPERTY_CATEGORY:
> - event->category = element->integer.value;
> + event->category = value;
> break;
>
> default:
> @@ -1511,8 +1603,8 @@ static void hp_wmi_notify(u32 value, void *context)
> struct acpi_buffer out = { ACPI_ALLOCATE_BUFFER, NULL };
> struct hp_wmi_sensors *state = context;
> struct device *dev = &state->wdev->dev;
> + struct hp_wmi_event event = {};
> struct hp_wmi_info *fan_info;
> - struct hp_wmi_event event;
> union acpi_object *wobj;
> acpi_status err;
> int event_type;
> @@ -1546,7 +1638,7 @@ static void hp_wmi_notify(u32 value, void *context)
>
> wobj = out.pointer;
>
> - err = populate_event_from_wobj(&event, wobj);
> + err = populate_event_from_wobj(dev, &event, wobj);
> if (err) {
> dev_warn(dev, "Bad event data (ACPI type %d)\n", wobj->type);
> goto out_free_wobj;
> @@ -1577,6 +1669,9 @@ static void hp_wmi_notify(u32 value, void *context)
> out_free_wobj:
> kfree(wobj);
>
> + devm_kfree(dev, event.name);
> + devm_kfree(dev, event.description);
> +
> out_unlock:
> mutex_unlock(&state->lock);
> }

Next message: Guenter Roeck: "Re: [PATCH v1 2/3] hwmon: tmp513: Simplify with dev_err_probe()"
Previous message: Like Xu: "Re: [PATCH] KVM: x86/pmu: Prevent any host user from enabling PEBS for profiling guest"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]