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

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

[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@linux.intel.com>
---
 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++;
-- 
2.40.0.1.gaa8946217a0b

[PATCH v1 2/2] platform/x86: wmi: Replace open coded guid_parse_and_compare()

[Andy Shevchenko]

Even though we have no issues in the code, let's replace the open
coded guid_parse_and_compare().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/platform/x86/wmi.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
index 098512a53170..a78ddd83cda0 100644
--- a/drivers/platform/x86/wmi.c
+++ b/drivers/platform/x86/wmi.c
@@ -1241,11 +1241,7 @@ static bool guid_already_parsed_for_legacy(struct acpi_device *device, const gui
 	list_for_each_entry(wblock, &wmi_block_list, list) {
 		/* skip warning and register if we know the driver will use struct wmi_driver */
 		for (int i = 0; allow_duplicates[i] != NULL; i++) {
-			guid_t tmp;
-
-			if (guid_parse(allow_duplicates[i], &tmp))
-				continue;
-			if (guid_equal(&tmp, guid))
+			if (guid_parse_and_compare(allow_duplicates[i], guid))
 				return false;
 		}
 		if (guid_equal(&wblock->gblock.guid, guid)) {
-- 
2.40.0.1.gaa8946217a0b

Re: [PATCH v1 2/2] platform/x86: wmi: Replace open coded guid_parse_and_compare()

[Armin Wolf]

Am 21.06.23 um 17:11 schrieb Andy Shevchenko:

> Even though we have no issues in the code, let's replace the open
> coded guid_parse_and_compare().
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/platform/x86/wmi.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/platform/x86/wmi.c b/drivers/platform/x86/wmi.c
> index 098512a53170..a78ddd83cda0 100644
> --- a/drivers/platform/x86/wmi.c
> +++ b/drivers/platform/x86/wmi.c
> @@ -1241,11 +1241,7 @@ static bool guid_already_parsed_for_legacy(struct acpi_device *device, const gui
>   	list_for_each_entry(wblock, &wmi_block_list, list) {
>   		/* skip warning and register if we know the driver will use struct wmi_driver */
>   		for (int i = 0; allow_duplicates[i] != NULL; i++) {
> -			guid_t tmp;
> -
> -			if (guid_parse(allow_duplicates[i], &tmp))
> -				continue;
> -			if (guid_equal(&tmp, guid))
> +			if (guid_parse_and_compare(allow_duplicates[i], guid))
>   				return false;
>   		}
>   		if (guid_equal(&wblock->gblock.guid, guid)) {

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

Armin Wolf

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

Armin Wolf

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

[Barnabás Pőcze]

Hi


2023. június 21., szerda 23:20 keltezéssel, Armin Wolf <W_Armin@gmx.de> írta:

> [...]
> > @@ -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.
> [...]

Wouldn't it be better to change `__wmi_driver_register()` to check that?


Regards,
Barnabás Pőcze

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

[Armin Wolf]

Am 21.06.23 um 23:29 schrieb Barnabás Pőcze:

> Hi
>
>
> 2023. június 21., szerda 23:20 keltezéssel, Armin Wolf <W_Armin@gmx.de> írta:
>
>> [...]
>>> @@ -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.
>> [...]
> Wouldn't it be better to change `__wmi_driver_register()` to check that?
>
>
> Regards,
> Barnabás Pőcze

Good point, i guess we can just forget this idea. The original motivation for it was the WARN_ON()
inside wmi_dev_match(), but your right that this is the wrong place to check the GUID formating.

Armin Wolf

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

[Andy Shevchenko]

On Wed, Jun 21, 2023 at 11:50:51PM +0200, Armin Wolf wrote:
> Am 21.06.23 um 23:29 schrieb Barnabás Pőcze:
> > 2023. június 21., szerda 23:20 keltezéssel, Armin Wolf <W_Armin@gmx.de> írta:

[...]

> > > > -		if (WARN_ON(guid_parse(id->guid_string, &driver_guid)))
> > > 
> > > 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.

[...]

> > Wouldn't it be better to change `__wmi_driver_register()` to check that?
> 
> Good point, i guess we can just forget this idea. The original motivation for
> it was the WARN_ON() inside wmi_dev_match(), but your right that this is the
> wrong place to check the GUID formating.

I'm not sure what do you want me to do since patches are tested already.

I think that WARN_ON() is a bit bogus. First of all, it can be easily
transformed to BUG()-equivalent with panic_on_oops and hence kill the
entire system. If we need the message about wrong GUID format, it should
be done elsewhere (modpost ?). I.o.w. we shan't expect that code,
controlled by us, shoots to our foot.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Thu, Jun 22, 2023 at 11:43:20AM +0300, Andy Shevchenko wrote:
> On Wed, Jun 21, 2023 at 11:50:51PM +0200, Armin Wolf wrote:

...

> I think that WARN_ON() is a bit bogus. First of all, it can be easily
> transformed to BUG()-equivalent with panic_on_oops and hence kill the
> entire system. If we need the message about wrong GUID format, it should
> be done elsewhere (modpost ?). I.o.w. we shan't expect that code,
> controlled by us, shoots to our foot.

Additional info. There will be another driver elsewhere that may use similar
API and also needs GUID in device ID table.

Looking into that implementation it seems that validation should be made in
file2alias.c for WMI and reused by that driver.

So, taking into account that we have no wrong IDs so far, I would drop
WARN_ON() here and guarantee that file2alias.c will be changed to validate
the GUID one way or the other.

Would it work? Hans, what is your comment here?

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi Andy,

On 6/22/23 17:00, Andy Shevchenko wrote:
> On Thu, Jun 22, 2023 at 11:43:20AM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 21, 2023 at 11:50:51PM +0200, Armin Wolf wrote:
> 
> ...
> 
>> I think that WARN_ON() is a bit bogus. First of all, it can be easily
>> transformed to BUG()-equivalent with panic_on_oops and hence kill the
>> entire system. If we need the message about wrong GUID format, it should
>> be done elsewhere (modpost ?). I.o.w. we shan't expect that code,
>> controlled by us, shoots to our foot.
> 
> Additional info. There will be another driver elsewhere that may use similar
> API and also needs GUID in device ID table.
> 
> Looking into that implementation it seems that validation should be made in
> file2alias.c for WMI and reused by that driver.
> 
> So, taking into account that we have no wrong IDs so far, I would drop
> WARN_ON() here and guarantee that file2alias.c will be changed to validate
> the GUID one way or the other.
> 
> Would it work? Hans, what is your comment here?


I agree that warning on malformed GUIDs does not belong here and
your patch already drops the WARN_ON while switching to the new
guid_parse_and_compare() helper.

So I'll go and merge this into my fixes branch once rc1 is out.

Regards,

Hans

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

[Andy Shevchenko]

On Tue, Jul 04, 2023 at 01:02:11PM +0200, Hans de Goede wrote:
> On 6/22/23 17:00, Andy Shevchenko wrote:
> > On Thu, Jun 22, 2023 at 11:43:20AM +0300, Andy Shevchenko wrote:
> >> On Wed, Jun 21, 2023 at 11:50:51PM +0200, Armin Wolf wrote:

...

> >> I think that WARN_ON() is a bit bogus. First of all, it can be easily
> >> transformed to BUG()-equivalent with panic_on_oops and hence kill the
> >> entire system. If we need the message about wrong GUID format, it should
> >> be done elsewhere (modpost ?). I.o.w. we shan't expect that code,
> >> controlled by us, shoots to our foot.
> > 
> > Additional info. There will be another driver elsewhere that may use similar
> > API and also needs GUID in device ID table.
> > 
> > Looking into that implementation it seems that validation should be made in
> > file2alias.c for WMI and reused by that driver.
> > 
> > So, taking into account that we have no wrong IDs so far, I would drop
> > WARN_ON() here and guarantee that file2alias.c will be changed to validate
> > the GUID one way or the other.
> > 
> > Would it work? Hans, what is your comment here?
> 
> 
> I agree that warning on malformed GUIDs does not belong here and
> your patch already drops the WARN_ON while switching to the new
> guid_parse_and_compare() helper.
> 
> So I'll go and merge this into my fixes branch once rc1 is out.

Thank you!

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Wed, Jun 21, 2023 at 11:20:04PM +0200, Armin Wolf wrote:
> Am 21.06.23 um 17:11 schrieb Andy Shevchenko:

...

> > -		if (WARN_ON(guid_parse(id->guid_string, &driver_guid)))
> 
> 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.

Commented on that separately.

...

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

Thank you for testing!

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

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