linux-kernel - Re: [BUG] [BISECTED] [CORRECTION] 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: <c8316d69-1aa4-f911-8074-11193d9f1a7c@alu.unizg.hr>
Date:   Wed, 29 Mar 2023 18:27:31 +0200
From:   Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
To:     Hans de Goede <hdegoede@...hat.com>,
        Thomas Weißschuh <thomas@...ch.de>
Cc:     Armin Wolf <W_Armin@....de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, Mark Gross <markgross@...nel.org>,
        platform-driver-x86@...r.kernel.org,
        Mark Pearson <mpearson-lenovo@...ebb.ca>
Subject: Re: [BUG] [BISECTED] [CORRECTION] systemd-devd triggers kernel
 memleak apparently in drivers/core/dd.c: driver_register()

On 29.3.2023. 17:46, Hans de Goede wrote:

>> So, the "tlmi_setting" memory leak appears to be fixed by this diff.
>>
>> 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);
>>
>> 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.

Not at all. It was a very interesting problem and a great session of brainstorming with
Greg, Armin and Thomas.

> I assume that these fixes will be posted as a proper 3 patch
> patch-series (one patch per fix) once you are done testing?

This is for others to decide. Armin gave a great hint with the one patch, and I sort of
had a race condition of the one with Thomas that was the final fix ;-)

The tlmi_setting return value fix by Thomas is already committed to the for-next tree,
I got it from there.

I do not have any authorship pretensions, this was a collaboration, so I don't know what
is due in the Code of Conduct for such circumstances. I trust you guys will do the right thing.

The 6.3.0-rc4-00034-gfcd476ea6a88 kernel apparently has a stackdump:

  WARNING: CPU: 4 PID: 746 at drivers/thermal/thermal_sysfs.c:879 cooling_device_stats_setup+0xb4/0xc0

but with our without the patch, and it apparently isn't related.

Best regards,
Mirsad

-- 
Mirsad Todorovac
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb
Republic of Croatia, the European Union

Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu