| Message ID | 20230331180912.38392-1-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [v2] platform/x86: think-lmi: Fix memory leak when showing current settings | expand |
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
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
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
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
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 >
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
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 --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; }
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