Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel memleak apparently in drivers/core/dd.c: driver_register()

[Mark Pearson]

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@xxxxxxxxxxxx>:
>>>
<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