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

["Mark Pearson" <mpearson-lenovo@squebb.ca>]

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?
+               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