From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
To: Armin Wolf <W_Armin@gmx.de>, markpearson@lenovo.com
Cc: hdegoede@redhat.com, markgross@kernel.org, thomas@t-8ch.de,
gregkh@linuxfoundation.org, rafael@kernel.org,
platform-driver-x86@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86: think-lmi: Fix memory leak when showing current settings
Date: Fri, 31 Mar 2023 22:23:33 +0200 [thread overview]
Message-ID: <baa37033-9b0c-5e40-41a9-bca0836c1330@alu.unizg.hr> (raw)
In-Reply-To: <20230331180912.38392-1-W_Armin@gmx.de>
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
--
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union
"I see something approaching fast ... Will it be friends with me?"
next prev parent reply other threads:[~2023-03-31 20:23 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-31 18:09 [PATCH v2] platform/x86: think-lmi: Fix memory leak when showing current settings Armin Wolf
2023-03-31 19:14 ` Mark Pearson
2023-03-31 21:20 ` Armin Wolf
2023-03-31 19:34 ` Mirsad Goran Todorovac
2023-03-31 21:26 ` Armin Wolf
2023-03-31 20:23 ` Mirsad Goran Todorovac [this message]
2023-03-31 21:30 ` Armin Wolf
2023-04-01 2:54 ` Mirsad Goran Todorovac
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=baa37033-9b0c-5e40-41a9-bca0836c1330@alu.unizg.hr \
--to=mirsad.todorovac@alu.unizg.hr \
--cc=W_Armin@gmx.de \
--cc=gregkh@linuxfoundation.org \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=markgross@kernel.org \
--cc=markpearson@lenovo.com \
--cc=platform-driver-x86@vger.kernel.org \
--cc=rafael@kernel.org \
--cc=thomas@t-8ch.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).