diff mbox series

[v2] platform/x86: think-lmi: Fix memory leak when showing current settings

Message ID 20230331180912.38392-1-W_Armin@gmx.de
State New
Headers
Series [v2] platform/x86: think-lmi: Fix memory leak when showing current settings |

Commit Message

Armin Wolf March 31, 2023, 6:09 p.m. UTC 
  When retriving a item string with tlmi_setting(), the result has to be
freed using kfree(). In current_value_show() however, malformed
item strings are not freed, causing a memory leak.
Fix this by eliminating the early return responsible for this.

Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
Signed-off-by: Armin Wolf <W_Armin@gmx.de>
---
Changes in v2:
- Add Reported-by: and Link: tags
---
 drivers/platform/x86/think-lmi.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

--
2.30.2

Comments

Mark Pearson March 31, 2023, 7:14 p.m. UTC | #1 
Hi Armin

On Fri, Mar 31, 2023, at 2:09 PM, Armin Wolf wrote:
> When retriving a item string with tlmi_setting(), the result has to be
> freed using kfree(). In current_value_show() however, malformed
> item strings are not freed, causing a memory leak.
> Fix this by eliminating the early return responsible for this.
>
> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Link: 
> https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface 
> support on Lenovo platforms")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Changes in v2:
> - Add Reported-by: and Link: tags
> ---
>  drivers/platform/x86/think-lmi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c 
> b/drivers/platform/x86/think-lmi.c
> index cc66f7cbccf2..8cafb9d4016c 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject 
> *kobj, struct kobj_attribute *a
>  	/* validate and split from `item,value` -> `value` */
>  	value = strpbrk(item, ",");
>  	if (!value || value == item || !strlen(value + 1))
> -		return -EINVAL;
> +		ret = -EINVAL;
> +	else
> +		ret = sysfs_emit(buf, "%s\n", value + 1);
>
> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>  	kfree(item);
> +
>  	return ret;
>  }
>
> --
> 2.30.2

Thanks for doing this - it was on my todo list but you beat me to it.

As a minor note - the Fixes tag should, I think, be
Fixes: 0fdf10e5fc96 ("platform/x86: think-lmi: Split current_value to reflect only the value")

As that's when I believe I introduced the issue.

Mark
Mirsad Todorovac March 31, 2023, 7:34 p.m. UTC | #2 
On 31. 03. 2023. 20:09, Armin Wolf wrote:
> When retriving a item string with tlmi_setting(), the result has to be
> freed using kfree(). In current_value_show() however, malformed
> item strings are not freed, causing a memory leak.
> Fix this by eliminating the early return responsible for this.
> 
> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Changes in v2:
> - Add Reported-by: and Link: tags
> ---
>  drivers/platform/x86/think-lmi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index cc66f7cbccf2..8cafb9d4016c 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>  	/* validate and split from `item,value` -> `value` */
>  	value = strpbrk(item, ",");
>  	if (!value || value == item || !strlen(value + 1))
> -		return -EINVAL;
> +		ret = -EINVAL;
> +	else
> +		ret = sysfs_emit(buf, "%s\n", value + 1);
> 
> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>  	kfree(item);
> +
>  	return ret;
>  }

Hi, Armin,

You might have wanted it to be tested in the original setting?

Should this patch work as a standalone fix, without the others?

This part:

@@ -929,8 +929,10 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a

        /* validate and split from `item,value` -> `value` */
        value = strpbrk(item, ",");
-       if (!value || value == item || !strlen(value + 1))
+       if (!value || value == item || !strlen(value + 1)) {
+               kfree(item);
                return -EINVAL;
+       }

        ret = sysfs_emit(buf, "%s\n", value + 1);
        kfree(item);

was apparently superseded.

Should this one be applied? I guess it should, as I stated in email
<4dc118c2-0dde-bd5e-ea41-427ed33e4545@alu.unizg.hr> from 2023-03-29 20:49 UTC+02:

@@ -1457,10 +1458,10 @@ static int tlmi_analyze(void)
                         * name string.
                         * Try and pull that out if it's available.
                         */
-                       char *item, *optstart, *optend;
+                       char *optitem, *optstart, *optend;

-                       if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
-                               optstart = strstr(item, "[Optional:");
+                       if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
+                               optstart = strstr(optitem, "[Optional:");
                                if (optstart) {
                                        optstart += strlen("[Optional:");
                                        optend = strstr(optstart, "]");
@@ -1469,6 +1470,7 @@ static int tlmi_analyze(void)
                                                        kstrndup(optstart, optend - optstart,
                                                                        GFP_KERNEL);
                                }
+                               kfree(optitem);
                        }
                }
                /*

If Mark had found a better fix, then that one goes away, too.

NOTE PLEASE that in the above-mentioned message (like all the others) I just specified the
commit at which the test kernel was built + all the applied patches (git diff did not give
authors).

This did not imply that I claim Mr. Weißschuh's fix for tlmi_analyze() return, God forbid!
I apologise if I made room for such an impression.

That's all, I think. Thank Heavens. God bless!

I will assume the test build on the bottom patch + the Thomas's patch still apply + your patch.

Best regards,
Mirsad
Mirsad Todorovac March 31, 2023, 8:23 p.m. UTC | #3 
On 31. 03. 2023. 20:09, Armin Wolf wrote:
> When retriving a item string with tlmi_setting(), the result has to be
> freed using kfree(). In current_value_show() however, malformed
> item strings are not freed, causing a memory leak.
> Fix this by eliminating the early return responsible for this.
> 
> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
> ---
> Changes in v2:
> - Add Reported-by: and Link: tags
> ---
>  drivers/platform/x86/think-lmi.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index cc66f7cbccf2..8cafb9d4016c 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>  	/* validate and split from `item,value` -> `value` */
>  	value = strpbrk(item, ",");
>  	if (!value || value == item || !strlen(value + 1))
> -		return -EINVAL;
> +		ret = -EINVAL;
> +	else
> +		ret = sysfs_emit(buf, "%s\n", value + 1);
> 
> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>  	kfree(item);
> +
>  	return ret;
>  }
> 
> --
> 2.30.2

I can confirm that the test passed in the original environment that caused the kmemleak.

[root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak 
unreferenced object 0xffff8e614889e390 (size 16):
  comm "kworker/u12:5", pid 366, jiffies 4294896428 (age 93.704s)
  hex dump (first 16 bytes):
    6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
  backtrace:
    [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
    [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
    [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
    [<ffffffff86066a1a>] kstrdup+0x3a/0x70
    [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
    [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
    [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
    [<ffffffff8678ed17>] dev_set_name+0x57/0x80
    [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
    [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
    [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
    [<ffffffff85dd6dff>] kthread+0x10f/0x140
    [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
unreferenced object 0xffff8e6158f93b90 (size 16):
  comm "kworker/u12:5", pid 366, jiffies 4294896433 (age 93.684s)
  hex dump (first 16 bytes):
    6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
  backtrace:
    [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
    [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
    [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
    [<ffffffff86066a1a>] kstrdup+0x3a/0x70
    [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
    [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
    [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
    [<ffffffff8678ed17>] dev_set_name+0x57/0x80
    [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
    [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
    [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
    [<ffffffff85dd6dff>] kthread+0x10f/0x140
    [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
[root@pc-mtodorov marvin]# uname -rms
Linux 6.3.0-rc4-00034-gfcd476ea6a88-dirty x86_64
[root@pc-mtodorov marvin]# 

NOTE: The leaks here belong to drivers/memstick/core/memstick.c leak for which I have
proposed a fix in message <df560535-2a8e-de21-d45d-805159d70954@alu.unizg.hr>.

This test was built on the 6.3-rc4+ commit fcd476ea6a88 Torvalds tree + the following
patches (Armin's, and Thomas's).

 drivers/platform/x86/think-lmi.c | 18 ++++++++++--------
 drivers/usb/host/xhci.c          |  1 +
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c816646eb661..c2146add88ab 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
        /* validate and split from `item,value` -> `value` */
        value = strpbrk(item, ",");
        if (!value || value == item || !strlen(value + 1))
-               return -EINVAL;
+               ret = -EINVAL;
+       else
+               ret = sysfs_emit(buf, "%s\n", value + 1);
 
-       ret = sysfs_emit(buf, "%s\n", value + 1);
        kfree(item);
+
        return ret;
 }
 
@@ -1380,7 +1382,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
 
 static int tlmi_analyze(void)
 {
-       acpi_status status;
        int i, ret;
 
        if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) &&
@@ -1417,8 +1418,8 @@ static int tlmi_analyze(void)
                char *p;
 
                tlmi_priv.setting[i] = NULL;
-               status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
-               if (ACPI_FAILURE(status))
+               ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
+               if (ret)
                        break;
                if (!item)
                        break;
@@ -1457,10 +1458,10 @@ static int tlmi_analyze(void)
                         * name string.
                         * Try and pull that out if it's available.
                         */
-                       char *item, *optstart, *optend;
+                       char *optitem, *optstart, *optend;
 
-                       if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
-                               optstart = strstr(item, "[Optional:");
+                       if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
+                               optstart = strstr(optitem, "[Optional:");
                                if (optstart) {
                                        optstart += strlen("[Optional:");
                                        optend = strstr(optstart, "]");
@@ -1469,6 +1470,7 @@ static int tlmi_analyze(void)
                                                        kstrndup(optstart, optend - optstart,
                                                                        GFP_KERNEL);
                                }
+                               kfree(optitem);
                        }
                }
                /*
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6183ce8574b1..905f1e89ead8 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4438,6 +4438,7 @@ static int __maybe_unused xhci_change_max_exit_latency(struct xhci_hcd *xhci,
 
        if (!virt_dev || max_exit_latency == virt_dev->current_mel) {
                spin_unlock_irqrestore(&xhci->lock, flags);
+               xhci_free_command(xhci, command);
                return 0;
        }
 
Xhci patch from Mathias is included because it is well tested and already submitted and acked.

At your convenience and according to the Code of Conduct, you can add:

Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>

Otherwise, Armin, I think you should submit this patch rightly because all idea to search in
think-lmi.c was yours.

Bisect was also much faster and in fewer steps.

Thanks,
Mirsad
Armin Wolf March 31, 2023, 9:20 p.m. UTC | #4 
Am 31.03.23 um 21:14 schrieb Mark Pearson:

> Hi Armin
>
> On Fri, Mar 31, 2023, at 2:09 PM, Armin Wolf wrote:
>> When retriving a item string with tlmi_setting(), the result has to be
>> freed using kfree(). In current_value_show() however, malformed
>> item strings are not freed, causing a memory leak.
>> Fix this by eliminating the early return responsible for this.
>>
>> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>> Link:
>> https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
>> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface
>> support on Lenovo platforms")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Changes in v2:
>> - Add Reported-by: and Link: tags
>> ---
>>   drivers/platform/x86/think-lmi.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c
>> b/drivers/platform/x86/think-lmi.c
>> index cc66f7cbccf2..8cafb9d4016c 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject
>> *kobj, struct kobj_attribute *a
>>   	/* validate and split from `item,value` -> `value` */
>>   	value = strpbrk(item, ",");
>>   	if (!value || value == item || !strlen(value + 1))
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +	else
>> +		ret = sysfs_emit(buf, "%s\n", value + 1);
>>
>> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>>   	kfree(item);
>> +
>>   	return ret;
>>   }
>>
>> --
>> 2.30.2
> Thanks for doing this - it was on my todo list but you beat me to it.
>
> As a minor note - the Fixes tag should, I think, be
> Fixes: 0fdf10e5fc96 ("platform/x86: think-lmi: Split current_value to reflect only the value")
>
> As that's when I believe I introduced the issue.
>
> Mark

Hi,

you are correct, i will send a v3 soon.

Armin Wolf
Armin Wolf March 31, 2023, 9:26 p.m. UTC | #5 
Am 31.03.23 um 21:34 schrieb Mirsad Goran Todorovac:

> On 31. 03. 2023. 20:09, Armin Wolf wrote:
>> When retriving a item string with tlmi_setting(), the result has to be
>> freed using kfree(). In current_value_show() however, malformed
>> item strings are not freed, causing a memory leak.
>> Fix this by eliminating the early return responsible for this.
>>
>> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
>> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Changes in v2:
>> - Add Reported-by: and Link: tags
>> ---
>>   drivers/platform/x86/think-lmi.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index cc66f7cbccf2..8cafb9d4016c 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>   	/* validate and split from `item,value` -> `value` */
>>   	value = strpbrk(item, ",");
>>   	if (!value || value == item || !strlen(value + 1))
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +	else
>> +		ret = sysfs_emit(buf, "%s\n", value + 1);
>>
>> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>>   	kfree(item);
>> +
>>   	return ret;
>>   }
> Hi, Armin,
>
> You might have wanted it to be tested in the original setting?
>
> Should this patch work as a standalone fix, without the others?
>
> This part:
>
> @@ -929,8 +929,10 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>
>          /* validate and split from `item,value` -> `value` */
>          value = strpbrk(item, ",");
> -       if (!value || value == item || !strlen(value + 1))
> +       if (!value || value == item || !strlen(value + 1)) {
> +               kfree(item);
>                  return -EINVAL;
> +       }
>
>          ret = sysfs_emit(buf, "%s\n", value + 1);
>          kfree(item);
>
> was apparently superseded.

Hi,

this part is indeed superseded by the patch, and it should work as a standalone fix.
I thought it might be better to have two patches for those two memory leaks, as they
are not directly connected.

> Should this one be applied? I guess it should, as I stated in email
> <4dc118c2-0dde-bd5e-ea41-427ed33e4545@alu.unizg.hr> from 2023-03-29 20:49 UTC+02:
>
> @@ -1457,10 +1458,10 @@ static int tlmi_analyze(void)
>                           * name string.
>                           * Try and pull that out if it's available.
>                           */
> -                       char *item, *optstart, *optend;
> +                       char *optitem, *optstart, *optend;
>
> -                       if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> -                               optstart = strstr(item, "[Optional:");
> +                       if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
> +                               optstart = strstr(optitem, "[Optional:");
>                                  if (optstart) {
>                                          optstart += strlen("[Optional:");
>                                          optend = strstr(optstart, "]");
> @@ -1469,6 +1470,7 @@ static int tlmi_analyze(void)
>                                                          kstrndup(optstart, optend - optstart,
>                                                                          GFP_KERNEL);
>                                  }
> +                               kfree(optitem);
>                          }
>                  }
>                  /*
>
> If Mark had found a better fix, then that one goes away, too.
>
> NOTE PLEASE that in the above-mentioned message (like all the others) I just specified the
> commit at which the test kernel was built + all the applied patches (git diff did not give
> authors).
>
> This did not imply that I claim Mr. Weißschuh's fix for tlmi_analyze() return, God forbid!
> I apologise if I made room for such an impression.
>
> That's all, I think. Thank Heavens. God bless!
>
> I will assume the test build on the bottom patch + the Thomas's patch still apply + your patch.

All good.

Armin Wolf

> Best regards,
> Mirsad
>
Armin Wolf March 31, 2023, 9:30 p.m. UTC | #6 
Am 31.03.23 um 22:23 schrieb Mirsad Goran Todorovac:

> On 31. 03. 2023. 20:09, Armin Wolf wrote:
>> When retriving a item string with tlmi_setting(), the result has to be
>> freed using kfree(). In current_value_show() however, malformed
>> item strings are not freed, causing a memory leak.
>> Fix this by eliminating the early return responsible for this.
>>
>> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
>> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>> ---
>> Changes in v2:
>> - Add Reported-by: and Link: tags
>> ---
>>   drivers/platform/x86/think-lmi.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index cc66f7cbccf2..8cafb9d4016c 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>   	/* validate and split from `item,value` -> `value` */
>>   	value = strpbrk(item, ",");
>>   	if (!value || value == item || !strlen(value + 1))
>> -		return -EINVAL;
>> +		ret = -EINVAL;
>> +	else
>> +		ret = sysfs_emit(buf, "%s\n", value + 1);
>>
>> -	ret = sysfs_emit(buf, "%s\n", value + 1);
>>   	kfree(item);
>> +
>>   	return ret;
>>   }
>>
>> --
>> 2.30.2
> I can confirm that the test passed in the original environment that caused the kmemleak.
>
> [root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
> unreferenced object 0xffff8e614889e390 (size 16):
>    comm "kworker/u12:5", pid 366, jiffies 4294896428 (age 93.704s)
>    hex dump (first 16 bytes):
>      6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
>    backtrace:
>      [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>      [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>      [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
>      [<ffffffff86066a1a>] kstrdup+0x3a/0x70
>      [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
>      [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
>      [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
>      [<ffffffff8678ed17>] dev_set_name+0x57/0x80
>      [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
>      [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
>      [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
>      [<ffffffff85dd6dff>] kthread+0x10f/0x140
>      [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
> unreferenced object 0xffff8e6158f93b90 (size 16):
>    comm "kworker/u12:5", pid 366, jiffies 4294896433 (age 93.684s)
>    hex dump (first 16 bytes):
>      6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
>    backtrace:
>      [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>      [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>      [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
>      [<ffffffff86066a1a>] kstrdup+0x3a/0x70
>      [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
>      [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
>      [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
>      [<ffffffff8678ed17>] dev_set_name+0x57/0x80
>      [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
>      [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
>      [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
>      [<ffffffff85dd6dff>] kthread+0x10f/0x140
>      [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
> [root@pc-mtodorov marvin]# uname -rms
> Linux 6.3.0-rc4-00034-gfcd476ea6a88-dirty x86_64
> [root@pc-mtodorov marvin]#
>
> NOTE: The leaks here belong to drivers/memstick/core/memstick.c leak for which I have
> proposed a fix in message <df560535-2a8e-de21-d45d-805159d70954@alu.unizg.hr>.
>
> This test was built on the 6.3-rc4+ commit fcd476ea6a88 Torvalds tree + the following
> patches (Armin's, and Thomas's).
>
>   drivers/platform/x86/think-lmi.c | 18 ++++++++++--------
>   drivers/usb/host/xhci.c          |  1 +
>   2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
> index c816646eb661..c2146add88ab 100644
> --- a/drivers/platform/x86/think-lmi.c
> +++ b/drivers/platform/x86/think-lmi.c
> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>          /* validate and split from `item,value` -> `value` */
>          value = strpbrk(item, ",");
>          if (!value || value == item || !strlen(value + 1))
> -               return -EINVAL;
> +               ret = -EINVAL;
> +       else
> +               ret = sysfs_emit(buf, "%s\n", value + 1);
>
> -       ret = sysfs_emit(buf, "%s\n", value + 1);
>          kfree(item);
> +
>          return ret;
>   }
>
> @@ -1380,7 +1382,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
>
>   static int tlmi_analyze(void)
>   {
> -       acpi_status status;
>          int i, ret;
>
>          if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) &&
> @@ -1417,8 +1418,8 @@ static int tlmi_analyze(void)
>                  char *p;
>
>                  tlmi_priv.setting[i] = NULL;
> -               status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
> -               if (ACPI_FAILURE(status))
> +               ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
> +               if (ret)
>                          break;
>                  if (!item)
>                          break;
> @@ -1457,10 +1458,10 @@ static int tlmi_analyze(void)
>                           * name string.
>                           * Try and pull that out if it's available.
>                           */
> -                       char *item, *optstart, *optend;
> +                       char *optitem, *optstart, *optend;
>
> -                       if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
> -                               optstart = strstr(item, "[Optional:");
> +                       if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
> +                               optstart = strstr(optitem, "[Optional:");
>                                  if (optstart) {
>                                          optstart += strlen("[Optional:");
>                                          optend = strstr(optstart, "]");
> @@ -1469,6 +1470,7 @@ static int tlmi_analyze(void)
>                                                          kstrndup(optstart, optend - optstart,
>                                                                          GFP_KERNEL);
>                                  }
> +                               kfree(optitem);
>                          }
>                  }
>                  /*
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 6183ce8574b1..905f1e89ead8 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4438,6 +4438,7 @@ static int __maybe_unused xhci_change_max_exit_latency(struct xhci_hcd *xhci,
>
>          if (!virt_dev || max_exit_latency == virt_dev->current_mel) {
>                  spin_unlock_irqrestore(&xhci->lock, flags);
> +               xhci_free_command(xhci, command);
>                  return 0;
>          }
>
> Xhci patch from Mathias is included because it is well tested and already submitted and acked.
>
> At your convenience and according to the Code of Conduct, you can add:
>
> Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>
> Otherwise, Armin, I think you should submit this patch rightly because all idea to search in
> think-lmi.c was yours.
>
> Bisect was also much faster and in fewer steps.
>
> Thanks,
> Mirsad
>
Thank you for reporting the memory leak issues and testing the patches.
I will send a v3 of my patch soon which will contain your Tested-by: tag.

Armin Wolf
Mirsad Todorovac April 1, 2023, 2:54 a.m. UTC | #7 
On 31. 03. 2023. 23:30, Armin Wolf wrote:
> Am 31.03.23 um 22:23 schrieb Mirsad Goran Todorovac:
> 
>> On 31. 03. 2023. 20:09, Armin Wolf wrote:
>>> When retriving a item string with tlmi_setting(), the result has to be
>>> freed using kfree(). In current_value_show() however, malformed
>>> item strings are not freed, causing a memory leak.
>>> Fix this by eliminating the early return responsible for this.
>>>
>>> Reported-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>>> Link: https://lore.kernel.org/platform-driver-x86/01e920bc-5882-ba0c-dd15-868bf0eca0b8@alu.unizg.hr/T/#t
>>> Fixes: a40cd7ef22fb ("platform/x86: think-lmi: Add WMI interface support on Lenovo platforms")
>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de>
>>> ---
>>> Changes in v2:
>>> - Add Reported-by: and Link: tags
>>> ---
>>>   drivers/platform/x86/think-lmi.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index cc66f7cbccf2..8cafb9d4016c 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>>       /* validate and split from `item,value` -> `value` */
>>>       value = strpbrk(item, ",");
>>>       if (!value || value == item || !strlen(value + 1))
>>> -        return -EINVAL;
>>> +        ret = -EINVAL;
>>> +    else
>>> +        ret = sysfs_emit(buf, "%s\n", value + 1);
>>>
>>> -    ret = sysfs_emit(buf, "%s\n", value + 1);
>>>       kfree(item);
>>> +
>>>       return ret;
>>>   }
>>>
>>> -- 
>>> 2.30.2
>> I can confirm that the test passed in the original environment that caused the kmemleak.
>>
>> [root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak
>> unreferenced object 0xffff8e614889e390 (size 16):
>>    comm "kworker/u12:5", pid 366, jiffies 4294896428 (age 93.704s)
>>    hex dump (first 16 bytes):
>>      6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
>>    backtrace:
>>      [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>      [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>      [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
>>      [<ffffffff86066a1a>] kstrdup+0x3a/0x70
>>      [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
>>      [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
>>      [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
>>      [<ffffffff8678ed17>] dev_set_name+0x57/0x80
>>      [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
>>      [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
>>      [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
>>      [<ffffffff85dd6dff>] kthread+0x10f/0x140
>>      [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
>> unreferenced object 0xffff8e6158f93b90 (size 16):
>>    comm "kworker/u12:5", pid 366, jiffies 4294896433 (age 93.684s)
>>    hex dump (first 16 bytes):
>>      6d 65 6d 73 74 69 63 6b 30 00 cc cc cc cc cc cc  memstick0.......
>>    backtrace:
>>      [<ffffffff860fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>      [<ffffffff86102b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>      [<ffffffff860773c9>] __kmalloc_node_track_caller+0x59/0x180
>>      [<ffffffff86066a1a>] kstrdup+0x3a/0x70
>>      [<ffffffff86066a8c>] kstrdup_const+0x2c/0x40
>>      [<ffffffff864a987c>] kvasprintf_const+0x7c/0xb0
>>      [<ffffffff86e3b427>] kobject_set_name_vargs+0x27/0xa0
>>      [<ffffffff8678ed17>] dev_set_name+0x57/0x80
>>      [<ffffffffc0e49f0f>] memstick_check+0x10f/0x3b0 [memstick]
>>      [<ffffffff85dcb4c0>] process_one_work+0x250/0x530
>>      [<ffffffff85dcb7f8>] worker_thread+0x48/0x3a0
>>      [<ffffffff85dd6dff>] kthread+0x10f/0x140
>>      [<ffffffff85c02fa9>] ret_from_fork+0x29/0x50
>> [root@pc-mtodorov marvin]# uname -rms
>> Linux 6.3.0-rc4-00034-gfcd476ea6a88-dirty x86_64
>> [root@pc-mtodorov marvin]#
>>
>> NOTE: The leaks here belong to drivers/memstick/core/memstick.c leak for which I have
>> proposed a fix in message <df560535-2a8e-de21-d45d-805159d70954@alu.unizg.hr>.
>>
>> This test was built on the 6.3-rc4+ commit fcd476ea6a88 Torvalds tree + the following
>> patches (Armin's, and Thomas's).
>>
>>   drivers/platform/x86/think-lmi.c | 18 ++++++++++--------
>>   drivers/usb/host/xhci.c          |  1 +
>>   2 files changed, 11 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>> index c816646eb661..c2146add88ab 100644
>> --- a/drivers/platform/x86/think-lmi.c
>> +++ b/drivers/platform/x86/think-lmi.c
>> @@ -930,10 +930,12 @@ static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
>>          /* validate and split from `item,value` -> `value` */
>>          value = strpbrk(item, ",");
>>          if (!value || value == item || !strlen(value + 1))
>> -               return -EINVAL;
>> +               ret = -EINVAL;
>> +       else
>> +               ret = sysfs_emit(buf, "%s\n", value + 1);
>>
>> -       ret = sysfs_emit(buf, "%s\n", value + 1);
>>          kfree(item);
>> +
>>          return ret;
>>   }
>>
>> @@ -1380,7 +1382,6 @@ static struct tlmi_pwd_setting *tlmi_create_auth(const char *pwd_type,
>>
>>   static int tlmi_analyze(void)
>>   {
>> -       acpi_status status;
>>          int i, ret;
>>
>>          if (wmi_has_guid(LENOVO_SET_BIOS_SETTINGS_GUID) &&
>> @@ -1417,8 +1418,8 @@ static int tlmi_analyze(void)
>>                  char *p;
>>
>>                  tlmi_priv.setting[i] = NULL;
>> -               status = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
>> -               if (ACPI_FAILURE(status))
>> +               ret = tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID);
>> +               if (ret)
>>                          break;
>>                  if (!item)
>>                          break;
>> @@ -1457,10 +1458,10 @@ static int tlmi_analyze(void)
>>                           * name string.
>>                           * Try and pull that out if it's available.
>>                           */
>> -                       char *item, *optstart, *optend;
>> +                       char *optitem, *optstart, *optend;
>>
>> -                       if (!tlmi_setting(setting->index, &item, LENOVO_BIOS_SETTING_GUID)) {
>> -                               optstart = strstr(item, "[Optional:");
>> +                       if (!tlmi_setting(setting->index, &optitem, LENOVO_BIOS_SETTING_GUID)) {
>> +                               optstart = strstr(optitem, "[Optional:");
>>                                  if (optstart) {
>>                                          optstart += strlen("[Optional:");
>>                                          optend = strstr(optstart, "]");
>> @@ -1469,6 +1470,7 @@ static int tlmi_analyze(void)
>>                                                          kstrndup(optstart, optend - optstart,
>>                                                                          GFP_KERNEL);
>>                                  }
>> +                               kfree(optitem);
>>                          }
>>                  }
>>                  /*
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 6183ce8574b1..905f1e89ead8 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -4438,6 +4438,7 @@ static int __maybe_unused xhci_change_max_exit_latency(struct xhci_hcd *xhci,
>>
>>          if (!virt_dev || max_exit_latency == virt_dev->current_mel) {
>>                  spin_unlock_irqrestore(&xhci->lock, flags);
>> +               xhci_free_command(xhci, command);
>>                  return 0;
>>          }
>>
>> Xhci patch from Mathias is included because it is well tested and already submitted and acked.
>>
>> At your convenience and according to the Code of Conduct, you can add:
>>
>> Tested-by: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
>>
>> Otherwise, Armin, I think you should submit this patch rightly because all idea to search in
>> think-lmi.c was yours.
>>
>> Bisect was also much faster and in fewer steps.
>>
>> Thanks,
>> Mirsad
>>
> Thank you for reporting the memory leak issues and testing the patches.
> I will send a v3 of my patch soon which will contain your Tested-by: tag.

That's awesome, Armin.

I thought of a way to make an exact account of which patches were used in the build,}}}
i.e. adding patch checksum to 6.3.0-rc4-00034-gfcd476ea6a88-dirty, for currently in
rpm -ivh --oldpacage install the kernels

kernel-6.3.0_rc4mt+20230330_00051_g8bb95a1662f8_dirty-24.x86_64.rpm
kernel-6.3.0_rc4mt+20230330_00051_g8bb95a1662f8_dirty-25.x86_64.rpm
kernel-6.3.0_rc4mt+20230330_00051_g8bb95a1662f8_dirty-26.x86_64.rpm

all interlap, so I have to reboot in i.e. 6.1.15, remove the offending kernel, and
then CONFIG_LOCALVERSION_AUTO=y rpm build script should add something that rpm
command sees in the install process so the files do not overlap (kernel numbes
being truncated at '-' sign).

See what I mean?

Optionally, a /proc/<applied-patches-to-build> or something like that could be
added to the running kernel, much like i.e. TuxCare has kcarectl --patch-info
for live patches?

Tell me pls if I speak rubbish.

Regards,
Mirsad
diff mbox series

Patch

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index cc66f7cbccf2..8cafb9d4016c 100644
--- a/drivers/platform/x86/think-lmi.c
+++ b/drivers/platform/x86/think-lmi.c
@@ -930,10 +930,12 @@  static ssize_t current_value_show(struct kobject *kobj, struct kobj_attribute *a
 	/* validate and split from `item,value` -> `value` */
 	value = strpbrk(item, ",");
 	if (!value || value == item || !strlen(value + 1))
-		return -EINVAL;
+		ret = -EINVAL;
+	else
+		ret = sysfs_emit(buf, "%s\n", value + 1);

-	ret = sysfs_emit(buf, "%s\n", value + 1);
 	kfree(item);
+
 	return ret;
 }