Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <1dfb573a-7e60-2bad-00a1-132d37d05cfc@redhat.com>
Date:   Tue, 11 Jul 2023 11:36:20 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.12.0
Subject: Re: [PATCH v1 1/2] platform/x86: wmi: Break possible infinite loop
 when parsing GUID
To:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Cc:     Mark Gross <markgross@kernel.org>
References: <20230621151155.78279-1-andriy.shevchenko@linux.intel.com>
Content-Language: en-US, nl
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <20230621151155.78279-1-andriy.shevchenko@linux.intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

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@linux.intel.com>

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++;