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

[Mark Pearson]

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.

Thanks
Mark