lkml.org 
[lkml]   [2023]   [Mar]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
SubjectRe: [BUG] [RFC] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()
From
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 :(
>
>> 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

Here is the patch proposal according to what Mark advised (using different name for optitem):

diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c816646eb661..ab17254781c4 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);
@@ -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);
}
}
/*
I have tested it, but without a few blunders of my own.
I guess "nobody wins them all".

Best regards,
Mirsad

--
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
\
 
 \ /
  Last update: 2023-03-29 20:49    [W:0.115 / U:1.888 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site