[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <9fa34eb2-d119-9039-6db7-edfd0ee406f9@alu.unizg.hr>
Date: Wed, 29 Mar 2023 18:43:22 +0200
From: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>,
Hans de Goede <hdegoede@...hat.com>,
Thomas Weißschuh <thomas@...ch.de>
Cc: Armin Wolf <W_Armin@....de>, Greg KH <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
linux-kernel@...r.kernel.org,
"markgross@...nel.org" <markgross@...nel.org>,
"platform-driver-x86@...r.kernel.org"
<platform-driver-x86@...r.kernel.org>
Subject: Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel
memleak apparently in drivers/core/dd.c: driver_register()
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@....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
Powered by blists - more mailing lists