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: Windows password security audit tool. GUI, reports in PDF.

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

Message-Id: <a1896b4a-1843-4946-ab6f-63132a03e009@app.fastmail.com>
Date:   Fri, 31 Mar 2023 15:10:22 -0400
From:   "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To:     "Hans de Goede" <hdegoede@...hat.com>,
        "Mirsad Goran Todorovac" <mirsad.todorovac@....unizg.hr>,
        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()



On Fri, Mar 31, 2023, at 3:04 PM, Hans de Goede wrote:
> Hi,
>
> On 3/31/23 20:54, Mark Pearson wrote:
>> Hi all,
>> 
>> On Wed, Mar 29, 2023, at 5:50 PM, Mirsad Goran Todorovac wrote:
>>> On 29. 03. 2023. 21:21, Thomas Weißschuh wrote:
>>>>
>>>> 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
>>>
>>> Thank you, Thomas,
>>>
>>> Indeed, my mistake.
>>>
>>> I have accepted Armin's suggestion to test if that patch closed the leak, and I
>>> have just quoted it, never claiming authorship.
>>>
>>> I ought to apologise if I made confusion here.
>>>
>>> I was a bit euphoric about the leak being fixed, so forgive me for this blatant
>>> mistake. Of course, putting it here would cause a patch collision, so it was a
>>> stupid thing to do, and I would never do it in a formal patch submission ...
>>>
>>> Thanks, anyway for correction.
>>>
>>> Best regards,
>>> Mirsad
>>>
>> 
>> I have the patches ready to fix this issue - I just wanted to check that I wouldn't be stepping on anybodies toes or if there is a protocol for doing this.
>>  - I will add Reported-by tag for Mirsad and Suggested-by for Armin.
>>  - I've identified Fixes tags for the two commits that caused the issue.
>> Let me know if there's anything else I should do - otherwise I'll get them sent out ASAP.
>
> This sounds to me like you have covered all the bases.
>
> Note Armin did send out a related fix earlier today,
> which I guess is duplicate with one of your patches:
>
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20230331180912.38392-1-W_Armin@gmx.de/
>
> So maybe add Armin's patch on top of pdx86/fixes and
> use that as a base for your series (dropping your
> likely duplicate patch) ?
>
Makes sense - will do
Thanks!
Mark