From: Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>
To: "Mark Pearson" <mpearson-lenovo@squebb.ca>,
"Hans de Goede" <hdegoede@redhat.com>,
"Thomas Weißschuh" <thomas@t-8ch.de>
Cc: Armin Wolf <W_Armin@gmx.de>, Greg KH <gregkh@linuxfoundation.org>,
"Rafael J. Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org,
"markgross@kernel.org" <markgross@kernel.org>,
"platform-driver-x86@vger.kernel.org"
<platform-driver-x86@vger.kernel.org>
Subject: Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()
Date: Wed, 29 Mar 2023 18:43:22 +0200 [thread overview]
Message-ID: <9fa34eb2-d119-9039-6db7-edfd0ee406f9@alu.unizg.hr> (raw)
In-Reply-To: <9c142ac2-9340-4a9b-8541-99f613772340@app.fastmail.com>
On 29.3.2023. 18:24, Mark Pearson wrote:
> Hi
>
> On Wed, Mar 29, 2023, at 11:46 AM, Hans de Goede wrote:
>> Hi,
>>
>> On 3/29/23 16:18, Mirsad Goran Todorovac wrote:
>>> On 29.3.2023. 15:35, Thomas Weißschuh wrote:
>>>>
>>>> Mar 29, 2023 08:31:31 Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>:
>>>>
> <snip>
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index c816646eb661..e8c28f4f5a71 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -1469,6 +1469,7 @@ static int tlmi_analyze(void)
>>> kstrndup(optstart, optend - optstart,
>>> GFP_KERNEL);
>>> }
>>> + kfree(item);
>>> }
>>> }
>>> /*
>>>
>>> You were 3 minutes faster ;-)
>>>
>>> The build with this patch is finished. Apparently, that was the culprit, for now
> <snip>
>>>
>>>
>>> So, the "tlmi_setting" memory leak appears to be fixed by this diff.
>>>
> My only concern here is it looks like I was dumb and used the variable name 'item' twice in the same function. I guess the compiler is smart enough to handle it but I'd like to change the name to make it clearer which 'item' is being freed in each context.
>
> In that block I would change it to be:
> char *optitem, *optstart, *optend;
> and fix all the pieces in the block to then be correct too (with the needed free)
>
>>> The next step is to add Armin-suggested patch:
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index c816646eb661..1e77ecb0cba8 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -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);
>>>
> This looks good to me - thank you!
>
>>> and Thomas' correction for the return type of the tlmi_setting() function:
>>>
>>> diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
>>> index 86b33b74519be..c924e9e4a6a5b 100644
>>> --- a/drivers/platform/x86/think-lmi.c
>>> +++ b/drivers/platform/x86/think-lmi.c
>>> @@ -1353,7 +1353,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) &&
>>> @@ -1390,8 +1389,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;
>>>
>>> A build on top of 6.3-rc4+ fcd476ea6a88 commit is on the way, with all three included.
>>
>> Good work on catching these issues, thank you all for your work on this.
>>
> Seconded - thank you for flagging and catching this. These were my mistakes :(
Armin's hint was the largest part of the catch. I did not even suspect think-lmi.c,
to be honest ...
Apparently, the three patches together do not raise any new issues on my box, but of
course, proper testing and peer review is required.
Best regards,
Mirsad
>> I assume that these fixes will be posted as a proper 3 patch
>> patch-series (one patch per fix) once you are done testing?
>>
> Let me know if you are happy to propose the changes as a patch-series. If you don't have time I can help and get these in ASAP as I was the original culprit.
>
> Happy to help out with testing too as I have access to HW. Let me know.
>
> Mark
--
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
next prev parent reply other threads:[~2023-03-29 16:43 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <5059b11b-8b6e-394b-338f-49e1339067fa@alu.unizg.hr>
[not found] ` <ZCLPaYGKHlFQGKYQ@kroah.com>
2023-03-28 11:59 ` [BUG] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register() Mirsad Todorovac
2023-03-28 12:08 ` Mirsad Todorovac
2023-03-28 12:17 ` Greg Kroah-Hartman
2023-03-28 12:44 ` Mirsad Todorovac
2023-03-28 16:53 ` Armin Wolf
2023-03-28 19:06 ` Mirsad Goran Todorovac
2023-03-28 19:55 ` Armin Wolf
2023-03-29 8:13 ` Mirsad Goran Todorovac
2023-03-29 13:22 ` [BUG] [BISECTED] " Mirsad Goran Todorovac
2023-03-29 13:31 ` [BUG] [BISECTED] [CORRECTION] " Mirsad Goran Todorovac
2023-03-29 13:35 ` Thomas Weißschuh
2023-03-29 14:18 ` Mirsad Goran Todorovac
2023-03-29 15:46 ` Hans de Goede
2023-03-29 16:24 ` Mark Pearson
2023-03-29 16:43 ` Mirsad Goran Todorovac [this message]
2023-03-29 18:49 ` [BUG] [RFC] " Mirsad Goran Todorovac
2023-03-29 18:59 ` Mark Pearson
2023-03-29 19:21 ` Thomas Weißschuh
2023-03-29 21:50 ` Mirsad Goran Todorovac
2023-03-31 18:54 ` Mark Pearson
2023-03-31 19:04 ` Hans de Goede
2023-03-31 19:10 ` Mark Pearson
2023-03-31 19:13 ` Mirsad Goran Todorovac
2023-03-29 16:27 ` [BUG] [BISECTED] [CORRECTION] " 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=9fa34eb2-d119-9039-6db7-edfd0ee406f9@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=mpearson-lenovo@squebb.ca \
--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).