linux-kernel - Re: [linux-next][PATCH 1/1] gpio: Delete excess allocated label memory

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CAMRc=Mfb1Vg1qnwE7c95PBroDFySZ6c0b0HgUhyrHFFpCz0Diw@mail.gmail.com>
Date: Sat, 17 Feb 2024 10:52:03 -0800
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Xiaolei Wang <xiaolei.wang@...driver.com>
Cc: linus.walleij@...aro.org, andriy.shevchenko@...ux.intel.com, 
	linux-gpio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [linux-next][PATCH 1/1] gpio: Delete excess allocated label memory

On Sat, 17 Feb 2024 19:35:43 +0100, Bartosz Golaszewski <brgl@...ev.pl> said:
> On Sat, Feb 17, 2024 at 2:53 PM Xiaolei Wang <xiaolei.wang@...driver.com> wrote:
>>
>> The changes in commit 1f2bcb8c8ccd ("gpio: protect the
>> descriptor label with SRCU"), desc_set_label has already
>> allocated memory space for the label, so there is no need
>> to allocate it again. otherwise memory leaks will be
>> introduced.
>>
>
> No, we *want* to copy it if it's not in .rodata for the same reason we
> introduced SRCU. This may be a valid report but the fix is wrong.
>
>> unreferenced object 0xffff0000c3e4d0c0 (size 32):
>>   comm "kworker/u16:4", pid 60, jiffies 4294894555
>>   hex dump (first 32 bytes):
>>     72 65 67 75 6c 61 74 6f 72 2d 63 61 6e 32 2d 73  regulator-can2-s
>>     74 62 79 00 00 00 ff ff ff ff ff ff eb db ff ff  tby.............
>>   backtrace (crc 2c3a0350):
>>     [<00000000e93c5cf4>] kmemleak_alloc+0x34/0x40
>>     [<0000000097a2657f>] __kmalloc_node_track_caller+0x2c4/0x524
>>     [<000000000dd1c057>] kstrdup+0x4c/0x98
>>     [<00000000b513a96a>] kstrdup_const+0x34/0x40
>>     [<000000008a7f0feb>] gpiod_request_commit+0xdc/0x358
>>     [<00000000fc71ad64>] gpiod_request+0xd8/0x204
>>     [<00000000fa24b091>] gpiod_find_and_request+0x170/0x780
>>     [<0000000086ecf92d>] gpiod_get_index+0x70/0xe0
>>     [<000000004aef97f9>] gpiod_get_optional+0x18/0x30
>>     [<00000000312f1b25>] reg_fixed_voltage_probe+0x58c/0xad8
>>     [<00000000e6f47635>] platform_probe+0xc4/0x198
>>     [<00000000cf78fbdb>] really_probe+0x204/0x5a8
>>     [<00000000e28d05ec>] __driver_probe_device+0x158/0x2c4
>>     [<00000000e4fe452b>] driver_probe_device+0x60/0x18c
>>     [<00000000479fcf5d>] __device_attach_driver+0x168/0x208
>>     [<000000007d389f38>] bus_for_each_drv+0x104/0x190
>>
>
> Can you post the full kmemleak report for this, please?
>
> Bart
>

Ah, I think I see the problem. Can you test the following diff:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 02be0ba1a402..0fdd4ad242bd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -695,10 +695,15 @@ EXPORT_SYMBOL_GPL(gpiochip_line_is_valid);
 static void gpiodev_release(struct device *dev)
 {
 	struct gpio_device *gdev = to_gpio_device(dev);
+	struct gpio_desc *desc;
 	unsigned int i;

-	for (i = 0; i < gdev->ngpio; i++)
-		cleanup_srcu_struct(&gdev->descs[i].srcu);
+	for (i = 0; i < gdev->ngpio; i++) {
+		desc = &gdev->descs[i];
+
+		kfree_const(desc->label);
+		cleanup_srcu_struct(&desc->srcu);
+	}

 	ida_free(&gpio_ida, gdev->id);
 	kfree_const(gdev->label);

and let me know if it fixes the issue?

Bart

>> Fixes: 1f2bcb8c8ccd ("gpio: protect the descriptor label with SRCU")
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@...driver.com>
>> ---
>>  drivers/gpio/gpiolib.c | 6 ------
>>  1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index 02be0ba1a402..32191547dece 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2250,12 +2250,6 @@ static int gpiod_request_commit(struct gpio_desc *desc, const char *label)
>>         if (test_and_set_bit(FLAG_REQUESTED, &desc->flags))
>>                 return -EBUSY;
>>
>> -       if (label) {
>> -               label = kstrdup_const(label, GFP_KERNEL);
>> -               if (!label)
>> -                       return -ENOMEM;
>> -       }
>> -
>>         /* NOTE:  gpio_request() can be called in early boot,
>>          * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
>>          */
>> --
>> 2.25.1
>>
>