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: <bccb52fa-e1c9-482c-a024-9a02179728b2@t-8ch.de>
Date:   Wed, 29 Mar 2023 14:21:23 -0500 (EST)
From:   Thomas Weißschuh  <thomas@...ch.de>
To:     Mark Pearson <mpearson-lenovo@...ebb.ca>
Cc:     Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>,
        Hans de Goede <hdegoede@...hat.com>,
        Armin Wolf <W_Armin@....de>,
        Greg KH <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, markgross@...nel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [BUG] [RFC] systemd-devd triggers kernel memleak apparently in
 drivers/core/dd.c: driver_register()


Mar 29, 2023 14:00:22 Mark Pearson <mpearson-lenovo@...ebb.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?

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