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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <b06d1d1f-7cd5-4532-ac49-d449ef68bbcb@app.fastmail.com>
Date:   Wed, 29 Mar 2023 14:59:54 -0400
From:   "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To:     "Mirsad Goran Todorovac" <mirsad.todorovac@....unizg.hr>,
        "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] [RFC] systemd-devd triggers kernel memleak apparently in
 drivers/core/dd.c: driver_register()

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