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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxxxxx> 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().

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

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")

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

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






[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux