Re: [PATCH v1 1/2] platform/x86: wmi: Break possible infinite loop when parsing GUID

[Armin Wolf]

Am 21.06.23 um 17:11 schrieb Andy Shevchenko:

> The while-loop may break on one of the two conditions, either ID string
> is empty or GUID matches. The second one, may never be reached if the
> parsed string is not correct GUID. In such a case the loop will never
> advance to check the next ID.
>
> Break possible infinite loop by factoring out guid_parse_and_compare()
> helper which may be moved to the generic header for everyone later on
> and preventing from similar mistake in the future.
>
> Interestingly that firstly it appeared when WMI was turned into a bus
> driver, but later when duplicated GUIDs were checked, the while-loop
> has been replaced by for-loop and hence no mistake made again.
>
> Fixes: a48e23385fcf ("platform/x86: wmi: add context pointer field to struct wmi_device_id")
> Fixes: 844af950da94 ("platform/x86: wmi: Turn WMI into a bus driver")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>   drivers/platform/x86/wmi.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 5b95d7aa5c2f..098512a53170 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -136,6 +136,16 @@ static acpi_status find_guid(const char *guid_string, struct wmi_block **out)
>   	return AE_NOT_FOUND;
>   }
>
> +static bool guid_parse_and_compare(const char *string, const guid_t *guid)
> +{
> +	guid_t guid_input;
> +
> +	if (guid_parse(string, &guid_input))
> +		return false;
> +
> +	return guid_equal(&guid_input, guid);
> +}
> +
>   static const void *find_guid_context(struct wmi_block *wblock,
>   				     struct wmi_driver *wdriver)
>   {
> @@ -146,11 +156,7 @@ static const void *find_guid_context(struct wmi_block *wblock,
>   		return NULL;
>
>   	while (*id->guid_string) {
> -		guid_t guid_input;
> -
> -		if (guid_parse(id->guid_string, &guid_input))
> -			continue;
> -		if (guid_equal(&wblock->gblock.guid, &guid_input))
> +		if (guid_parse_and_compare(id->guid_string, &wblock->gblock.guid))
>   			return id->context;
>   		id++;
>   	}
> @@ -895,11 +901,7 @@ static int wmi_dev_match(struct device *dev, struct device_driver *driver)
>   		return 0;
>
>   	while (*id->guid_string) {
> -		guid_t driver_guid;
> -
> -		if (WARN_ON(guid_parse(id->guid_string, &driver_guid)))

Hi,

just an idea: how about printing an error/debug message in case of an malformed GUID?
This could be useful when searching for typos in GUIDs used by WMI drivers.

> -			continue;
> -		if (guid_equal(&driver_guid, &wblock->gblock.guid))
> +		if (guid_parse_and_compare(id->guid_string, &wblock->gblock.guid))
>   			return 1;
>
>   		id++;

Works on my Dell Inspiron 3505, so for this patch:
Tested-by: Armin Wolf <W_Armin@xxxxxx>

Armin Wolf