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

[Thomas Weißschuh]

Mar 29, 2023 14:00:22 Mark Pearson <mpearson-lenovo@xxxxxxxxx>:

> Thanks Mirsad
>
> On Wed, Mar 29, 2023, at 2:49 PM, Mirsad Goran Todorovac wrote:
> <snip>
>>
>> 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)
>
> Really minor, but tweak to be this and save a line of code?

This hunk is actually from another commit and should not be needed here.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/platform/x86/think-lmi.c?id=da62908efe80f132f691efc2ace4ca67626de86b

> +               if (tlmi_setting(i, &item, LENOVO_BIOS_SETTING_GUID))
>
>>                          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'm running a build locally and will aim to put this thru a few different platforms myself too and sanity check
>
>> I guess "nobody wins them all".
>
> I hear you :P
> > Mark