All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] platform/x86: think-lmi: Abort probe on analyze failure
@ 2021-11-08 18:03 Alex Williamson
  2021-11-08 18:24 ` [External] " Mark Pearson
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Alex Williamson @ 2021-11-08 18:03 UTC (permalink / raw)
  To: markpearson, hdegoede, markgross
  Cc: alex.williamson, platform-driver-x86, linux-kernel

A Lenovo ThinkStation S20 (4157CTO BIOS 60KT41AUS) fails to boot on
recent kernels including the think-lmi driver, due to the fact that
errors returned by the tlmi_analyze() function are ignored by
tlmi_probe(), where  tlmi_sysfs_init() is called unconditionally.
This results in making use of an array of already freed, non-null
pointers and other uninitialized globals, causing all sorts of nasty
kobject and memory faults.

Make use of the analyze function return value, free a couple leaked
allocations, and remove the settings_count field, which is incremented
but never consumed.

Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 drivers/platform/x86/think-lmi.c |   13 ++++++++++---
 drivers/platform/x86/think-lmi.h |    1 -
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index 9472aae72df2..c4d9c45350f7 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -888,8 +888,10 @@ static int tlmi_analyze(void)
 			break;
 		if (!item)
 			break;
-		if (!*item)
+		if (!*item) {
+			kfree(item);
 			continue;
+		}
 
 		/* It is not allowed to have '/' for file name. Convert it into '\'. */
 		strreplace(item, '/', '\\');
@@ -902,6 +904,7 @@ static int tlmi_analyze(void)
 		setting = kzalloc(sizeof(*setting), GFP_KERNEL);
 		if (!setting) {
 			ret = -ENOMEM;
+			kfree(item);
 			goto fail_clear_attr;
 		}
 		setting->index = i;
@@ -916,7 +919,6 @@ static int tlmi_analyze(void)
 		}
 		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
 		tlmi_priv.setting[i] = setting;
-		tlmi_priv.settings_count++;
 		kfree(item);
 	}
 
@@ -983,7 +985,12 @@ static void tlmi_remove(struct wmi_device *wdev)
 
 static int tlmi_probe(struct wmi_device *wdev, const void *context)
 {
-	tlmi_analyze();
+	int ret;
+
+	ret = tlmi_analyze();
+	if (ret)
+		return ret;
+
 	return tlmi_sysfs_init();
 }
 
diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
index f8e26823075f..2ce5086a5af2 100644
--- a/drivers/platform/x86/think-lmi.h
+++ b/drivers/platform/x86/think-lmi.h
@@ -55,7 +55,6 @@ struct tlmi_attr_setting {
 struct think_lmi {
 	struct wmi_device *wmi_device;
 
-	int settings_count;
 	bool can_set_bios_settings;
 	bool can_get_bios_selections;
 	bool can_set_bios_password;



^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [External] [PATCH] platform/x86: think-lmi: Abort probe on analyze failure
  2021-11-08 18:03 [PATCH] platform/x86: think-lmi: Abort probe on analyze failure Alex Williamson
@ 2021-11-08 18:24 ` Mark Pearson
  2021-11-08 19:38 ` Mark Gross
  2021-11-09  9:26 ` Hans de Goede
  2 siblings, 0 replies; 4+ messages in thread
From: Mark Pearson @ 2021-11-08 18:24 UTC (permalink / raw)
  To: Alex Williamson, hdegoede, markgross; +Cc: platform-driver-x86, linux-kernel


Thanks Alex

On 2021-11-08 13:03, Alex Williamson wrote:
> A Lenovo ThinkStation S20 (4157CTO BIOS 60KT41AUS) fails to boot on
> recent kernels including the think-lmi driver, due to the fact that
> errors returned by the tlmi_analyze() function are ignored by
> tlmi_probe(), where  tlmi_sysfs_init() is called unconditionally.
> This results in making use of an array of already freed, non-null
> pointers and other uninitialized globals, causing all sorts of nasty
> kobject and memory faults.
> 
> Make use of the analyze function return value, free a couple leaked
> allocations, and remove the settings_count field, which is incremented
> but never consumed.
> 
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>   drivers/platform/x86/think-lmi.c |   13 ++++++++++---
>   drivers/platform/x86/think-lmi.h |    1 -
>   2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 9472aae72df2..c4d9c45350f7 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -888,8 +888,10 @@ static int tlmi_analyze(void)
>   			break;
>   		if (!item)
>   			break;
> -		if (!*item)
> +		if (!*item) {
> +			kfree(item);
>   			continue;
> +		}
I feel bad that I missed this in my original commit.
I've (obviously) not seen it on any platforms I've tested. I'll see if
we can get the FW team to address the origins of this too - I'm guessing
the root cause is in the FW

>   
>   		/* It is not allowed to have '/' for file name. Convert it into '\'. */
>   		strreplace(item, '/', '\\');
> @@ -902,6 +904,7 @@ static int tlmi_analyze(void)
>   		setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>   		if (!setting) {
>   			ret = -ENOMEM;
> +			kfree(item);
>   			goto fail_clear_attr;
>   		}
>   		setting->index = i;
> @@ -916,7 +919,6 @@ static int tlmi_analyze(void)
>   		}
>   		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>   		tlmi_priv.setting[i] = setting;
> -		tlmi_priv.settings_count++;
>   		kfree(item);
>   	}
>   
> @@ -983,7 +985,12 @@ static void tlmi_remove(struct wmi_device *wdev)
>   
>   static int tlmi_probe(struct wmi_device *wdev, const void *context)
>   {
> -	tlmi_analyze();
> +	int ret;
> +
> +	ret = tlmi_analyze();
> +	if (ret)
> +		return ret;
> +
>   	return tlmi_sysfs_init();
>   }
Looks good

>   
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index f8e26823075f..2ce5086a5af2 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -55,7 +55,6 @@ struct tlmi_attr_setting {
>   struct think_lmi {
>   	struct wmi_device *wmi_device;
>   
> -	int settings_count;
>   	bool can_set_bios_settings;
>   	bool can_get_bios_selections;
>   	bool can_set_bios_password;
> 
Originally the settings_count was used in the ioctl based driver we 
have, but I have no issue with it being removed as it serves no use. 
Thanks for the clean up.

Reviewed-by: Mark Pearson <markpearson@lenovo.com>

Mark

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Abort probe on analyze failure
  2021-11-08 18:03 [PATCH] platform/x86: think-lmi: Abort probe on analyze failure Alex Williamson
  2021-11-08 18:24 ` [External] " Mark Pearson
@ 2021-11-08 19:38 ` Mark Gross
  2021-11-09  9:26 ` Hans de Goede
  2 siblings, 0 replies; 4+ messages in thread
From: Mark Gross @ 2021-11-08 19:38 UTC (permalink / raw)
  To: Alex Williamson
  Cc: markpearson, hdegoede, markgross, platform-driver-x86, linux-kernel

On Mon, Nov 08, 2021 at 11:03:57AM -0700, Alex Williamson wrote:
> A Lenovo ThinkStation S20 (4157CTO BIOS 60KT41AUS) fails to boot on
> recent kernels including the think-lmi driver, due to the fact that
> errors returned by the tlmi_analyze() function are ignored by
> tlmi_probe(), where  tlmi_sysfs_init() is called unconditionally.
> This results in making use of an array of already freed, non-null
> pointers and other uninitialized globals, causing all sorts of nasty
> kobject and memory faults.
> 
> Make use of the analyze function return value, free a couple leaked
> allocations, and remove the settings_count field, which is incremented
> but never consumed.
part of me would like to see this split into 2 patches, one to fix the memory
leaks, and one to fix the boot issue.  But, its so small its hard to argue for
splitting up.

Looks good enough to me:

Reviewed-by: Mark Gross <markgross@kernel.org>

> 
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  drivers/platform/x86/think-lmi.c |   13 ++++++++++---
>  drivers/platform/x86/think-lmi.h |    1 -
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 9472aae72df2..c4d9c45350f7 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -888,8 +888,10 @@ static int tlmi_analyze(void)
>  			break;
>  		if (!item)
>  			break;
> -		if (!*item)
> +		if (!*item) {
> +			kfree(item);
>  			continue;
> +		}
>  
>  		/* It is not allowed to have '/' for file name. Convert it into '\'. */
>  		strreplace(item, '/', '\\');
> @@ -902,6 +904,7 @@ static int tlmi_analyze(void)
>  		setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>  		if (!setting) {
>  			ret = -ENOMEM;
> +			kfree(item);
>  			goto fail_clear_attr;
>  		}
>  		setting->index = i;
> @@ -916,7 +919,6 @@ static int tlmi_analyze(void)
>  		}
>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
> -		tlmi_priv.settings_count++;
>  		kfree(item);
>  	}
>  
> @@ -983,7 +985,12 @@ static void tlmi_remove(struct wmi_device *wdev)
>  
>  static int tlmi_probe(struct wmi_device *wdev, const void *context)
>  {
> -	tlmi_analyze();
> +	int ret;
> +
> +	ret = tlmi_analyze();
> +	if (ret)
> +		return ret;
> +
>  	return tlmi_sysfs_init();
>  }
>  
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index f8e26823075f..2ce5086a5af2 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -55,7 +55,6 @@ struct tlmi_attr_setting {
>  struct think_lmi {
>  	struct wmi_device *wmi_device;
>  
> -	int settings_count;
>  	bool can_set_bios_settings;
>  	bool can_get_bios_selections;
>  	bool can_set_bios_password;
> 
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread
* Re: [PATCH] platform/x86: think-lmi: Abort probe on analyze failure
  2021-11-08 18:03 [PATCH] platform/x86: think-lmi: Abort probe on analyze failure Alex Williamson
  2021-11-08 18:24 ` [External] " Mark Pearson
  2021-11-08 19:38 ` Mark Gross
@ 2021-11-09  9:26 ` Hans de Goede
  2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2021-11-09  9:26 UTC (permalink / raw)
  To: Alex Williamson, markpearson, markgross; +Cc: platform-driver-x86, linux-kernel

Hi,

On 11/8/21 19:03, Alex Williamson wrote:
> A Lenovo ThinkStation S20 (4157CTO BIOS 60KT41AUS) fails to boot on
> recent kernels including the think-lmi driver, due to the fact that
> errors returned by the tlmi_analyze() function are ignored by
> tlmi_probe(), where  tlmi_sysfs_init() is called unconditionally.
> This results in making use of an array of already freed, non-null
> pointers and other uninitialized globals, causing all sorts of nasty
> kobject and memory faults.
> 
> Make use of the analyze function return value, free a couple leaked
> allocations, and remove the settings_count field, which is incremented
> but never consumed.
> 
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

I'll send this to Linus in my first fixes pull-req for 5.16
(once 516-rc1 is out)

Regards,

Hans



> ---
>  drivers/platform/x86/think-lmi.c |   13 ++++++++++---
>  drivers/platform/x86/think-lmi.h |    1 -
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index 9472aae72df2..c4d9c45350f7 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -888,8 +888,10 @@ static int tlmi_analyze(void)
>  			break;
>  		if (!item)
>  			break;
> -		if (!*item)
> +		if (!*item) {
> +			kfree(item);
>  			continue;
> +		}
>  
>  		/* It is not allowed to have '/' for file name. Convert it into '\'. */
>  		strreplace(item, '/', '\\');
> @@ -902,6 +904,7 @@ static int tlmi_analyze(void)
>  		setting = kzalloc(sizeof(*setting), GFP_KERNEL);
>  		if (!setting) {
>  			ret = -ENOMEM;
> +			kfree(item);
>  			goto fail_clear_attr;
>  		}
>  		setting->index = i;
> @@ -916,7 +919,6 @@ static int tlmi_analyze(void)
>  		}
>  		kobject_init(&setting->kobj, &tlmi_attr_setting_ktype);
>  		tlmi_priv.setting[i] = setting;
> -		tlmi_priv.settings_count++;
>  		kfree(item);
>  	}
>  
> @@ -983,7 +985,12 @@ static void tlmi_remove(struct wmi_device *wdev)
>  
>  static int tlmi_probe(struct wmi_device *wdev, const void *context)
>  {
> -	tlmi_analyze();
> +	int ret;
> +
> +	ret = tlmi_analyze();
> +	if (ret)
> +		return ret;
> +
>  	return tlmi_sysfs_init();
>  }
>  
> diff --git a/drivers/platform/x86/think-lmi.h b/drivers/platform/x86/think-lmi.h
> index f8e26823075f..2ce5086a5af2 100644
> --- a/drivers/platform/x86/think-lmi.h
> +++ b/drivers/platform/x86/think-lmi.h
> @@ -55,7 +55,6 @@ struct tlmi_attr_setting {
>  struct think_lmi {
>  	struct wmi_device *wmi_device;
>  
> -	int settings_count;
>  	bool can_set_bios_settings;
>  	bool can_get_bios_selections;
>  	bool can_set_bios_password;
> 
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-11-09  9:26 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-08 18:03 [PATCH] platform/x86: think-lmi: Abort probe on analyze failure Alex Williamson
2021-11-08 18:24 ` [External] " Mark Pearson
2021-11-08 19:38 ` Mark Gross
2021-11-09  9:26 ` Hans de Goede

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.