| 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
| ||
|
Date: Sun, 18 Feb 2024 08:55:05 +0800
From: xiaolei wang <xiaolei.wang@...driver.com>
To: Bartosz Golaszewski <brgl@...ev.pl>
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 2/18/24 02:52, Bartosz Golaszewski wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> 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?
I tried the following modifications, which did not fix the problem,
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 02be0ba1a402..5940ef88399c 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -695,10 +695,14 @@ 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);
unreferenced object 0xffff0000c0e83740 (size 32):
comm "kworker/u16:0", pid 10, jiffies 4294894561
hex dump (first 32 bytes):
72 65 67 75 6c 61 74 6f 72 2d 75 73 64 68 63 32 regulator-usdhc2
00 37 e8 c0 00 00 ff ff 00 00 00 00 00 00 00 00 .7..............
backtrace (crc 222fadd4):
[<00000000cd106dac>] kmemleak_alloc+0x34/0x40
[<00000000e084cf5f>] __kmalloc_node_track_caller+0x2c4/0x524
[<00000000746173b0>] kstrdup+0x4c/0x98
[<00000000f79b3bf4>] kstrdup_const+0x34/0x40
[<00000000f47eb728>] gpiod_request_commit+0xdc/0x358
[<000000003a6899af>] gpiod_request+0xd8/0x204
[<00000000c93118be>] gpiod_find_and_request+0x170/0x780
[<00000000660eebdd>] gpiod_get_index+0x70/0xe0
[<00000000eb599be3>] gpiod_get_optional+0x18/0x30
[<00000000e8b3cfcb>] reg_fixed_voltage_probe+0x58c/0xad8
[<000000009737579a>] platform_probe+0xc4/0x198
[<00000000405158e3>] really_probe+0x204/0x5a8
[<00000000772dd378>] __driver_probe_device+0x158/0x2c4
[<00000000ddc25aba>] driver_probe_device+0x60/0x18c
[<00000000cc4413ff>] __device_attach_driver+0x168/0x208
[<00000000e04f3755>] bus_for_each_drv+0x104/0x190
I'm a little confused. In the gpiod_request_commit() function, if label
is true, we allocate space kstrdup_const() for it, but desc_set_label()
will still be called. If label is true, space will be allocated to it
again, desc-> label will be pointed to the place where new is allocated,
thanks
xiaolei
>
> 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
>>>
Powered by blists - more mailing lists