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

[Mirsad Goran Todorovac]

On 31. 03. 2023. 21:10, Mark Pearson wrote:
> 
> 
> 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@xxxxxxxxx>:
>>>>>
>>>>>> 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@xxxxxx/
>>
>> 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

Hi, Mark,

You might find it convenient to test the patches in my initial environment that triggered
the bug. Otherwise, it is fine with me.

Regards,
Mirsad

-- 
Mirsad Goran Todorovac
Sistem inženjer
Grafički fakultet | Akademija likovnih umjetnosti
Sveučilište u Zagrebu
 
System engineer
Faculty of Graphic Arts | Academy of Fine Arts
University of Zagreb, Republic of Croatia
The European Union

"I see something approaching fast ... Will it be friends with me?"