Hi,
On 6/21/23 17:11, Andy Shevchenko wrote:
> 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>
Thank you for your series, I've applied this series to my fixes
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=fixes
I will include this patch in my next fixes pull-req to Linus
for the current kernel development cycle.
Regards,
Hans
> ---
> 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)))
> - continue;
> - if (guid_equal(&driver_guid, &wblock->gblock.guid))
> + if (guid_parse_and_compare(id->guid_string, &wblock->gblock.guid))
> return 1;
>
> id++;
