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

[Mirsad Goran Todorovac <mirsad.todorovac@alu.unizg.hr>]

On 28.3.2023. 21:55, Armin Wolf wrote:
> Am 28.03.23 um 21:06 schrieb Mirsad Goran Todorovac:
> 
>> On 3/28/2023 6:53 PM, Armin Wolf wrote:
>>> Am 28.03.23 um 14:44 schrieb Mirsad Todorovac:
>>>
>>>> On 3/28/23 14:17, Greg Kroah-Hartman wrote:
>>>>> On Tue, Mar 28, 2023 at 02:08:06PM +0200, Mirsad Todorovac wrote:
>>>>>> On 3/28/23 13:59, Mirsad Todorovac wrote:
>>>>>>
>>>>>>> On 3/28/23 13:28, Greg Kroah-Hartman wrote:
>>>>>>>> On Tue, Mar 28, 2023 at 01:13:33PM +0200, Mirsad Todorovac wrote:
>>>>>>>>> Hi all,
>>>>>>>>>
>>>>>>>>> Here is another kernel memory leak report, just as I thought we
>>>>>>>>> have done with
>>>>>>>>> them by the xhci patch by Mathias.
>>>>>>>>>
>>>>>>>>> The memory leaks were caught on an AlmaLinux 8.7 (CentOS) fork
>>>>>>>>> system, running
>>>>>>>>> on a Lenovo desktop box (see lshw.txt) and the newest Linux
>>>>>>>>> kernel 6.3-rc4 commit
>>>>>>>>> g3a93e40326c8 with Mathias' patch for a xhci systemd-devd
>>>>>>>>> triggered leak.
>>>>>>>>>
>>>>>>>>>           See:
>>>>>>>>> <20230327095019.1017159-1-mathias.nyman@linux.intel.com> on LKML.
>>>>>>>>>
>>>>>>>>> This leak is also systemd-devd triggered, except for the
>>>>>>>>> memstick_check() leaks
>>>>>>>>> which I was unable to bisect due to the box not booting older
>>>>>>>>> kernels (work in
>>>>>>>>> progress).
>>>>>>>>>
>>>>>>>>> unreferenced object 0xffff88ad12392710 (size 96):
>>>>>>>>>     comm "systemd-udevd", pid 735, jiffies 4294896759 (age
>>>>>>>>> 2257.568s)
>>>>>>>>>     hex dump (first 32 bytes):
>>>>>>>>>       53 65 72 69 61 6c 50 6f 72 74 31 41 64 64 72 65
>>>>>>>>> SerialPort1Addre
>>>>>>>>>       73 73 2c 33 46 38 2f 49 52 51 34 3b 5b 4f 70 74
>>>>>>>>> ss,3F8/IRQ4;[Opt
>>>>>>>>>     backtrace:
>>>>>>>>>       [<ffffffffae8fb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>>>>>>>>       [<ffffffffae902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>>>>>>>>       [<ffffffffae8773c9>] __kmalloc_node_track_caller+0x59/0x180
>>>>>>>>>       [<ffffffffae866a1a>] kstrdup+0x3a/0x70
>>>>>>>>>       [<ffffffffc0d839aa>]
>>>>>>>>> tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi]
>>>>>>>>>       [<ffffffffc0d83b64>] tlmi_setting.constprop.4+0x54/0x90
>>>>>>>>> [think_lmi]
>>>>>>>>>       [<ffffffffc0d842b1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>>>>>>>>       [<ffffffffc051dc53>] wmi_dev_probe+0x163/0x230 [wmi]
>>>>>>>>
>>> Hi,
>>>
>>> this "SerialPort1Address" string looks like a BIOS setup option, and
>>> indeed think_lmi allows for
>>> changing BIOS setup options over sysfs. While looking at
>>> current_value_show() in think-lmi.c, i noticed
>>> that "item" holds a string which is allocated with kstrdup(), so it
>>> has to be freed using kfree().
>>> This however does not happen if strbrk() fails, so maybe the memory
>>> leak is caused by this?
>>>
>>> Armin Wolf
>>
>> Hi Armin,
>>
>> I tried your suggestion, and though it is an obvious improvement and a
>> leak fix, this
>> was not the one we were searching for.
>>
>> I tested the following 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);
>>
>> (I would also object to the use of strlen() here, for it is inherently
>> insecure
>> against SEGFAULT in kernel space.)
>>
>> I still get:
>> [root@pc-mtodorov marvin]# uname -rms
>> Linux 6.3.0-rc4-armin-patch-00025-g3a93e40326c8-dirty x86_64
>> [root@pc-mtodorov marvin]# cat /sys/kernel/debug/kmemleak [edited]
>> unreferenced object 0xffff8eb008ef9260 (size 96):
>>   comm "systemd-udevd", pid 771, jiffies 4294896499 (age 74.880s)
>>   hex dump (first 32 bytes):
>>     53 65 72 69 61 6c 50 6f 72 74 31 41 64 64 72 65 SerialPort1Addre
>>     73 73 2c 33 46 38 2f 49 52 51 34 3b 5b 4f 70 74 ss,3F8/IRQ4;[Opt
>>   backtrace:
>>     [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>     [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>     [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>     [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>     [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>> [think_lmi]
>>     [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>     [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>     [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>     [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>     [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>     [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>     [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>     [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>     [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>     [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>     [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>> unreferenced object 0xffff8eb018ddbb40 (size 64):
>>   comm "systemd-udevd", pid 771, jiffies 4294896528 (age 74.780s)
>>   hex dump (first 32 bytes):
>>     55 53 42 50 6f 72 74 41 63 63 65 73 73 2c 45 6e USBPortAccess,En
>>     61 62 6c 65 64 3b 5b 4f 70 74 69 6f 6e 61 6c 3a abled;[Optional:
>>   backtrace:
>>     [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>     [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>     [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>     [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>     [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>> [think_lmi]
>>     [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>     [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>     [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>     [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>     [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>     [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>     [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>     [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>     [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>     [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>     [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>> unreferenced object 0xffff8eb006fe2b40 (size 64):
>>   comm "systemd-udevd", pid 771, jiffies 4294896542 (age 74.724s)
>>   hex dump (first 32 bytes):
>>     55 53 42 42 49 4f 53 53 75 70 70 6f 72 74 2c 45 USBBIOSSupport,E
>>     6e 61 62 6c 65 64 3b 5b 4f 70 74 69 6f 6e 61 6c nabled;[Optional
>>   backtrace:
>>     [<ffffffff9eafb26c>] slab_post_alloc_hook+0x8c/0x3e0
>>     [<ffffffff9eb02b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
>>     [<ffffffff9ea773c9>] __kmalloc_node_track_caller+0x59/0x180
>>     [<ffffffff9ea66a1a>] kstrdup+0x3a/0x70
>>     [<ffffffffc0eef9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60
>> [think_lmi]
>>     [<ffffffffc0eefb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
>>     [<ffffffffc0ef02c1>] tlmi_probe+0x591/0xba0 [think_lmi]
>>     [<ffffffffc0629c53>] wmi_dev_probe+0x163/0x230 [wmi]
>>     [<ffffffff9f1987eb>] really_probe+0x17b/0x3d0
>>     [<ffffffff9f198ad4>] __driver_probe_device+0x84/0x190
>>     [<ffffffff9f198c14>] driver_probe_device+0x24/0xc0
>>     [<ffffffff9f198ed2>] __driver_attach+0xc2/0x190
>>     [<ffffffff9f195ab1>] bus_for_each_dev+0x81/0xd0
>>     [<ffffffff9f197c62>] driver_attach+0x22/0x30
>>     [<ffffffff9f197354>] bus_add_driver+0x1b4/0x240
>>     [<ffffffff9f19a0a2>] driver_register+0x62/0x120
>>
>> There are currently 84 wmi_dev_probe leaks, sized mostly 64 bytes, and
>> one 96 and two 192 bytes.
>>
>> I also cannot figure out the mechanism by which current_value_show()
>> is called, when it is static?
>>
>> Any idea?
>>
>> Thanks.
>>
>> Best regards,
>> Mirsad
>>
> Can you tell me how many BIOS settings think-lmi provides on your machine? Because according to the stacktrace,
> the other place where the leak could have occurred is inside tlmi_analyze(), which calls tlmi_setting().

Yes, Sir!

I think these could be the ones you need (totaling 83, which is close to 82 systemd-udevd leaks):

[root@pc-mtodorov marvin]# ls -l /sys/devices/virtual/firmware-attributes/thinklmi/attributes | grep ^d
drwxr-xr-x 2 root root    0 Mar 29 08:24 AfterPowerLoss
drwxr-xr-x 2 root root    0 Mar 29 08:24 AlarmDate(MM\DD\YYYY)
drwxr-xr-x 2 root root    0 Mar 29 08:24 AlarmDayofWeek
drwxr-xr-x 2 root root    0 Mar 29 08:24 AlarmTime(HH:MM:SS)
drwxr-xr-x 2 root root    0 Mar 29 08:24 ASPMSupport
drwxr-xr-x 2 root root    0 Mar 29 08:24 AutomaticBootSequence
drwxr-xr-x 2 root root    0 Mar 29 08:24 BIOSPasswordAtBootDeviceList
drwxr-xr-x 2 root root    0 Mar 29 08:24 BIOSPasswordAtReboot
drwxr-xr-x 2 root root    0 Mar 29 08:24 BIOSPasswordAtUnattendedBoot
drwxr-xr-x 2 root root    0 Mar 29 08:24 BootMode
drwxr-xr-x 2 root root    0 Mar 29 08:24 BootPriority
drwxr-xr-x 2 root root    0 Mar 29 08:24 BootUpNum-LockStatus
drwxr-xr-x 2 root root    0 Mar 29 08:24 C1ESupport
drwxr-xr-x 2 root root    0 Mar 29 08:24 CardReader
drwxr-xr-x 2 root root    0 Mar 29 08:24 ClearTCGSecurityFeature
drwxr-xr-x 2 root root    0 Mar 29 08:24 ComputraceModuleActivation
drwxr-xr-x 2 root root    0 Mar 29 08:24 ConfigurationChangeDetection
drwxr-xr-x 2 root root    0 Mar 29 08:24 ConfigureSATAas
drwxr-xr-x 2 root root    0 Mar 29 08:24 CoreMulti-Processing
drwxr-xr-x 2 root root    0 Mar 29 08:24 CoverTamperDetected
drwxr-xr-x 2 root root    0 Mar 29 08:24 CSM
drwxr-xr-x 2 root root    0 Mar 29 08:24 CStateSupport
drwxr-xr-x 2 root root    0 Mar 29 08:24 DeviceGuard
drwxr-xr-x 2 root root    0 Mar 29 08:24 DustShieldAlert
drwxr-xr-x 2 root root    0 Mar 29 08:24 EISTSupport
drwxr-xr-x 2 root root    0 Mar 29 08:24 EnhancedPowerSavingMode
drwxr-xr-x 2 root root    0 Mar 29 08:24 ErrorBootSequence
drwxr-xr-x 2 root root    0 Mar 29 08:24 Friday
drwxr-xr-x 2 root root    0 Mar 29 08:24 FrontUSBPorts
drwxr-xr-x 2 root root    0 Mar 29 08:24 HardDiskPre-delay
drwxr-xr-x 2 root root    0 Mar 29 08:24 Intel(R)SGXControl
drwxr-xr-x 2 root root    0 Mar 29 08:24 InternalSpeaker
drwxr-xr-x 2 root root    0 Mar 29 08:24 Monday
drwxr-xr-x 2 root root    0 Mar 29 08:24 OnboardAudioController
drwxr-xr-x 2 root root    0 Mar 29 08:24 OnboardEthernetController
drwxr-xr-x 2 root root    0 Mar 29 08:24 OptionKeysDisplay
drwxr-xr-x 2 root root    0 Mar 29 08:24 OptionKeysDisplayStyle
drwxr-xr-x 2 root root    0 Mar 29 08:24 OSOptimizedDefaults
drwxr-xr-x 2 root root    0 Mar 29 08:24 PasswordCountExceededError
drwxr-xr-x 2 root root    0 Mar 29 08:24 PCIe16xSlotSpeed
drwxr-xr-x 2 root root    0 Mar 29 08:24 PCIe1xSlot1Speed
drwxr-xr-x 2 root root    0 Mar 29 08:24 PrimaryBootSequence
drwxr-xr-x 2 root root    0 Mar 29 08:24 PXEIPV4NetworkStack
drwxr-xr-x 2 root root    0 Mar 29 08:24 PXEIPV6NetworkStack
drwxr-xr-x 2 root root    0 Mar 29 08:24 PXEOptionROM
drwxr-xr-x 2 root root    0 Mar 29 08:24 RearUSBPorts
drwxr-xr-x 2 root root    0 Mar 29 08:24 RequireAdmin.Pass.whenFlashing
drwxr-xr-x 2 root root    0 Mar 29 08:24 RequireHDPonSystemBoot
drwxr-xr-x 2 root root    0 Mar 29 08:24 SATAController
drwxr-xr-x 2 root root    0 Mar 29 08:24 SATADrive1
drwxr-xr-x 2 root root    0 Mar 29 08:24 SATADrive2
drwxr-xr-x 2 root root    0 Mar 29 08:24 Saturday
drwxr-xr-x 2 root root    0 Mar 29 08:24 SecureBoot
drwxr-xr-x 2 root root    0 Mar 29 08:24 SecureRollBackPrevention
drwxr-xr-x 2 root root    0 Mar 29 08:24 SecurityChip
drwxr-xr-x 2 root root    0 Mar 29 08:24 SelectActiveVideo
drwxr-xr-x 2 root root    0 Mar 29 08:24 SerialPort1Address
drwxr-xr-x 2 root root    0 Mar 29 08:24 SmartUSBProtection
drwxr-xr-x 2 root root    0 Mar 29 08:24 StartupDeviceMenuPrompt
drwxr-xr-x 2 root root    0 Mar 29 08:24 StartupSequence
drwxr-xr-x 2 root root    0 Mar 29 08:24 Sunday
drwxr-xr-x 2 root root    0 Mar 29 08:24 Thursday
drwxr-xr-x 2 root root    0 Mar 29 08:24 Tuesday
drwxr-xr-x 2 root root    0 Mar 29 08:24 TurboMode
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBBIOSSupport
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBEnumerationDelay
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort1
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort2
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort3
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort4
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort5
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort6
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort7
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPort8
drwxr-xr-x 2 root root    0 Mar 29 08:24 USBPortAccess
drwxr-xr-x 2 root root    0 Mar 29 08:24 UserDefinedAlarmTime(HH:MM:SS)
drwxr-xr-x 2 root root    0 Mar 29 08:24 VirtualizationTechnology
drwxr-xr-x 2 root root    0 Mar 29 08:24 VTdFeature
drwxr-xr-x 2 root root    0 Mar 29 08:24 WakefromSerialPortRing
drwxr-xr-x 2 root root    0 Mar 29 08:24 WakeOnLAN
drwxr-xr-x 2 root root    0 Mar 29 08:24 WakeUponAlarm
drwxr-xr-x 2 root root    0 Mar 29 08:24 Wednesday
drwxr-xr-x 2 root root    0 Mar 29 08:24 WindowsUEFIFirmwareUpdate
[root@pc-mtodorov marvin]# ls -l /sys/devices/virtual/firmware-attributes/thinklmi/attributes | grep ^d | wc -l
83
[root@pc-mtodorov marvin]#

> However, i have no idea on how *info is somehow leaked, it has to happen inside the for-loop tween the call
> to tlmi_setting() and strreplace(), because otherwise the strings would not contain the "/" character.

I see. It is the line 1404.

> Can you check if the problem is somehow solved by applying the following commit from the platform-drivers-x86
> for-next branch:
> da62908efe80 ("platform/x86: think-lmi: Properly interpret return value of tlmi_setting")

It could possibly be that. But I do not recall seeing these messages in 6.3-rc3 ...

...

Unfortunately, the build with Thomas' patch you referred to did not work:

unreferenced object 0xffff9dff4d28bbc8 (size 192):
   comm "systemd-udevd", pid 769, jiffies 4294897473 (age 85.700s)
   hex dump (first 32 bytes):
     50 72 69 6d 61 72 79 42 6f 6f 74 53 65 71 75 65  PrimaryBootSeque
     6e 63 65 2c 4d 2e 32 20 44 72 69 76 65 20 31 3a  nce,M.2 Drive 1:
   backtrace:
     [<ffffffffa48fb26c>] slab_post_alloc_hook+0x8c/0x3e0
     [<ffffffffa4902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
     [<ffffffffa48773c9>] __kmalloc_node_track_caller+0x59/0x180
     [<ffffffffa4866a1a>] kstrdup+0x3a/0x70
     [<ffffffffc0c7f9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi]
     [<ffffffffc0c7fb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
     [<ffffffffc0c802c1>] tlmi_probe+0x591/0xba0 [think_lmi]
     [<ffffffffc03c9c53>] wmi_dev_probe+0x163/0x230 [wmi]
     [<ffffffffa4f987eb>] really_probe+0x17b/0x3d0
     [<ffffffffa4f98ad4>] __driver_probe_device+0x84/0x190
     [<ffffffffa4f98c14>] driver_probe_device+0x24/0xc0
     [<ffffffffa4f98ed2>] __driver_attach+0xc2/0x190
     [<ffffffffa4f95ab1>] bus_for_each_dev+0x81/0xd0
     [<ffffffffa4f97c62>] driver_attach+0x22/0x30
     [<ffffffffa4f97354>] bus_add_driver+0x1b4/0x240
     [<ffffffffa4f9a0a2>] driver_register+0x62/0x120
unreferenced object 0xffff9dff4d28a008 (size 192):
   comm "systemd-udevd", pid 769, jiffies 4294897517 (age 85.540s)
   hex dump (first 32 bytes):
     45 72 72 6f 72 42 6f 6f 74 53 65 71 75 65 6e 63  ErrorBootSequenc
     65 2c 4e 65 74 77 6f 72 6b 20 31 3a 4d 2e 32 20  e,Network 1:M.2
   backtrace:
     [<ffffffffa48fb26c>] slab_post_alloc_hook+0x8c/0x3e0
     [<ffffffffa4902b49>] __kmem_cache_alloc_node+0x1d9/0x2a0
     [<ffffffffa48773c9>] __kmalloc_node_track_caller+0x59/0x180
     [<ffffffffa4866a1a>] kstrdup+0x3a/0x70
     [<ffffffffc0c7f9aa>] tlmi_extract_output_string.isra.0+0x2a/0x60 [think_lmi]
     [<ffffffffc0c7fb64>] tlmi_setting.constprop.4+0x54/0x90 [think_lmi]
     [<ffffffffc0c802c1>] tlmi_probe+0x591/0xba0 [think_lmi]
     [<ffffffffc03c9c53>] wmi_dev_probe+0x163/0x230 [wmi]
     [<ffffffffa4f987eb>] really_probe+0x17b/0x3d0
     [<ffffffffa4f98ad4>] __driver_probe_device+0x84/0x190
     [<ffffffffa4f98c14>] driver_probe_device+0x24/0xc0
     [<ffffffffa4f98ed2>] __driver_attach+0xc2/0x190
     [<ffffffffa4f95ab1>] bus_for_each_dev+0x81/0xd0
     [<ffffffffa4f97c62>] driver_attach+0x22/0x30
     [<ffffffffa4f97354>] bus_add_driver+0x1b4/0x240
     [<ffffffffa4f9a0a2>] driver_register+0x62/0x120
[root@pc-mtodorov marvin]# uname -rms
Linux 6.3.0-rc4-armin+tw-patch-00025-g3a93e40326c8-dirty x86_64
[root@pc-mtodorov marvin]#

What was applied is:

mtodorov@domac:~/linux/kernel/linux_torvalds$ git diff
diff --git a/drivers/platform/x86/think-lmi.c b/drivers/platform/x86/think-lmi.c
index c816646eb661..9a3015f43aaf 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)
                         break;
                 if (!item)
                         break;


> Also current_value_show() is used by attr_current_val, the __ATTR_RW_MODE() macro arranges for that.

Thanks.

In this build:

[root@pc-mtodorov marvin]# uname -rms
Linux 6.3.0-rc34tests-00001-g6981739a967c x86_64
[root@pc-mtodorov marvin]#

... the bug isn't present, so it might be something added recently:

commit 8a02d70679fc1c434401863333c8ea7dbf201494
Author: Mark Pearson <mpearson-lenovo@squebb.ca>
Date:   Sun Mar 19 20:32:21 2023 -0400

     platform/x86: think-lmi: Add possible_values for ThinkStation

commit cf337f27f3bfc4aeab4954c468239fd6233c7638
Author: Mark Pearson <mpearson-lenovo@squebb.ca>
Date:   Sun Mar 19 20:32:20 2023 -0400

     platform/x86: think-lmi: only display possible_values if available

commit 45e21289bfc6e257885514790a8a8887da822d40
Author: Mark Pearson <mpearson-lenovo@squebb.ca>
Date:   Sun Mar 19 20:32:19 2023 -0400

     platform/x86: think-lmi: use correct possible_values delimiters

commit 583329dcf22e568a328a944f20427ccfc95dce01
Author: Mark Pearson <mpearson-lenovo@squebb.ca>
Date:   Sun Mar 19 20:32:18 2023 -0400

     platform/x86: think-lmi: add missing type attribute

I have CC:-ed the author of the commits.

I can try bisect, but only after my day job.

Have a nice day!

Best regards,
Mirsad

> Armin Wolf
> 
>>>>>>>> Why aren't you looking at the wmi.c driver?  That should be
>>>>>>>> where the
>>>>>>>> issue is, not the driver core, right?
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>>
>>>>>>>> greg k-h
>>>>>>>
>>>>>>> Hi, Mr. Greg,
>>>>>>>
>>>>>>> Thanks for the quick reply.
>>>>>>>
>>>>>>> I have added CC: for additional developers per
>>>>>>> drivers/platform/x86/wmi.c,
>>>>>>> however, this seems to me like hieroglyphs. There is nothing
>>>>>>> obvious, but
>>>>>>> I had not noticed it with v6.3-rc3?
>>>>>>>
>>>>>>> Maybe, there seems to be something off:
>>>>>>>
>>>>>>>       949 static int wmi_dev_probe(struct device *dev)
>>>>>>>       950 {
>>>>>>>       951         struct wmi_block *wblock = dev_to_wblock(dev);
>>>>>>>       952         struct wmi_driver *wdriver =
>>>>>>> drv_to_wdrv(dev->driver);
>>>>>>>       953         int ret = 0;
>>>>>>>       954         char *buf;
>>>>>>>       955
>>>>>>>       956         if (ACPI_FAILURE(wmi_method_enable(wblock, true)))
>>>>>>>       957                 dev_warn(dev, "failed to enable device
>>>>>>> -- probing anyway\n");
>>>>>>>       958
>>>>>>>       959         if (wdriver->probe) {
>>>>>>>       960                 ret = wdriver->probe(dev_to_wdev(dev),
>>>>>>>       961 find_guid_context(wblock, wdriver));
>>>>>>>       962                 if (ret != 0)
>>>>>>>       963                         goto probe_failure;
>>>>>>>       964         }
>>>>>>>       965
>>>>>>>       966         /* driver wants a character device made */
>>>>>>>       967         if (wdriver->filter_callback) {
>>>>>>>       968                 /* check that required buffer size
>>>>>>> declared by driver or MOF */
>>>>>>>       969                 if (!wblock->req_buf_size) {
>>>>>>>       970 dev_err(&wblock->dev.dev,
>>>>>>>       971                                 "Required buffer size
>>>>>>> not set\n");
>>>>>>>       972                         ret = -EINVAL;
>>>>>>>       973                         goto probe_failure;
>>>>>>>       974                 }
>>>>>>>       975
>>>>>>>       976                 wblock->handler_data =
>>>>>>> kmalloc(wblock->req_buf_size,
>>>>>>>       977 GFP_KERNEL);
>>>>>>>       978                 if (!wblock->handler_data) {
>>>>>>>       979                         ret = -ENOMEM;
>>>>>>>       980                         goto probe_failure;
>>>>>>>       981                 }
>>>>>>>       982
>>>>>>>       983                 buf = kasprintf(GFP_KERNEL, "wmi/%s",
>>>>>>> wdriver->driver.name);
>>>>>>>       984                 if (!buf) {
>>>>>>>       985                         ret = -ENOMEM;
>>>>>>>       986                         goto probe_string_failure;
>>>>>>>       987                 }
>>>>>>>       988                 wblock->char_dev.minor =
>>>>>>> MISC_DYNAMIC_MINOR;
>>>>>>>       989                 wblock->char_dev.name = buf;
>>>>>>>       990                 wblock->char_dev.fops = &wmi_fops;
>>>>>>>       991                 wblock->char_dev.mode = 0444;
>>>>>>>       992                 ret = misc_register(&wblock->char_dev);
>>>>>>>       993                 if (ret) {
>>>>>>>       994                         dev_warn(dev, "failed to
>>>>>>> register char dev: %d\n", ret);
>>>>>>>       995                         ret = -ENOMEM;
>>>>>>>       996                         goto probe_misc_failure;
>>>>>>>       997                 }
>>>>>>>       998         }
>>>>>>>       999
>>>>>>>      1000         set_bit(WMI_PROBED, &wblock->flags);
>>>>>>>      1001         return 0;
>>>>>>>      1002
>>>>>>>      1003 probe_misc_failure:
>>>>>>>      1004         kfree(buf);
>>>>>>>      1005 probe_string_failure:
>>>>>>>      1006         kfree(wblock->handler_data);
>>>>>>>      1007 probe_failure:
>>>>>>>      1008         if (ACPI_FAILURE(wmi_method_enable(wblock,
>>>>>>> false)))
>>>>>>>      1009                 dev_warn(dev, "failed to disable
>>>>>>> device\n");
>>>>>>>
>>>>>>>
>>>>>>> char *buf is passed to kfree(buf) uninitialised if
>>>>>>> wdriver->filter_callback
>>>>>>> is not set.
>>>>>>>
>>>>>>> It seems like a logical error per se, but I don't believe this is
>>>>>>> the cause
>>>>>>> of the leak?
>>>>>>
>>>>>> CORRECTION:
>>>>>>
>>>>>> I overlooked the "return 0" in line 1001.
>>>>>
>>>>> Yeah, and the memory looks to be freed properly in the
>>>>> wmi_dev_remove()
>>>>> callback, right?
>>>>
>>>> It would appear so. To verify that:
>>>>
>>>> Alloc:
>>>> 976        wblock->handler_data = kmalloc(wblock->req_buf_size,
>>>>                            GFP_KERNEL);
>>>>         <check>
>>>>
>>>> 983        buf = kasprintf(GFP_KERNEL, "wmi/%s", wdriver->driver.name);
>>>>         <check>
>>>> 989        wblock->char_dev.name = buf;
>>>>
>>>> In lines 1022-1023:
>>>>
>>>> 1022        kfree(wblock->char_dev.name);
>>>> 1023        kfree(wblock->handler_data);
>>>>
>>>>>> This is why I don't think things should be rushed, but analysed
>>>>>> with clear and
>>>>>> cold head. And with as many eyes as possible :)
>>>>>>
>>>>>> The driver stuff is my long-term research interest. To state the
>>>>>> obvious,
>>>>>> the printing and multimedia education and industry in general
>>>>>> would benefit from
>>>>>> the open-source drivers for many instruments that still work, but
>>>>>> are obsoleted
>>>>>> by the producer and require unsupported versions of the OS.
>>>>>>
>>>>>> Thank you again for reviewing the bug report, however, ATM I do
>>>>>> not think I have
>>>>>> what it takes to hunt down the memleak. :-/
>>>>>
>>>>> Do you have a reproducer that you can use to show the problem better?
>>>>
>>>> Unfortunately, the problem doesn't seem to appear during the run of
>>>> a particular
>>>> test, but immediately on startup of the OS. This makes it awkward to
>>>> pinpoint the
>>>> exact service that triggered memory leaks. But they would appear to
>>>> have to do
>>>> with the initialisation of the USB devices, wouldn't they?
>>>>
>>>> There seem to be strings:
>>>>
>>>> "USBPortAccess,Enabled;[Optional:"
>>>> "USBBIOSSupport,Enabled;[Optional"
>>>> "USBEnumerationDelay,Disabled;[Op"
>>>>
>>>> This seems to be happening during USB initialisation and before any
>>>> services.
>>>> But I might as well be wrong.
>>>>
>>>>> Or can you test kernel patches to verify the problem is fixed or
>>>>> not if
>>>>> we send you patches to test?
>>>>
>>>> Certainly, Lord willing, I can test the patches in the same
>>>> environment that
>>>> mainfeted the bug (or memleak).
>>>>
>>>> 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