| 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
| ||
|
Message-ID: <CAMRc=MeFGMtjoq4fwfJ17pBPAstOAjBi3fcQQrjJJnawzx7W5A@mail.gmail.com>
Date: Wed, 22 Feb 2023 11:53:35 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Mirsad Goran Todorovac <mirsad.todorovac@....unizg.hr>
Cc: linux-gpio@...r.kernel.org,
Linus Walleij <linus.walleij@...aro.org>,
linux-kernel@...r.kernel.org,
Thorsten Leemhuis <regressions@...mhuis.info>,
Andy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: INFO: REPRODUCED: memory leak in gpio device in 6.2-rc6
On Tue, Feb 21, 2023 at 4:41 PM Andy Shevchenko
<andriy.shevchenko@...el.com> wrote:
>
> On Tue, Feb 21, 2023 at 02:52:38PM +0100, Mirsad Goran Todorovac wrote:
> > On 20. 02. 2023. 14:43, Andy Shevchenko wrote:
> > > On Mon, Feb 20, 2023 at 02:10:00PM +0100, Mirsad Todorovac wrote:
> > > > On 2/16/23 15:16, Bartosz Golaszewski wrote:
>
> ...
>
> > > > As Mr. McKenney once said, a bunch of monkeys with keyboard could
> > > > have done it in a considerable number of trials and errors ;-)
> > > >
> > > > But here I have something that could potentially leak as well. I could not devise a
> > > > reproducer due to the leak being lightly triggered only in extreme memory contention.
> > > >
> > > > See it for yourself:
> > > >
> > > > drivers/gpio/gpio-sim.c:
> > > > 301 static int gpio_sim_setup_sysfs(struct gpio_sim_chip *chip)
> > > > 302 {
> > > > 303 struct device_attribute *val_dev_attr, *pull_dev_attr;
> > > > 304 struct gpio_sim_attribute *val_attr, *pull_attr;
> > > > 305 unsigned int num_lines = chip->gc.ngpio;
> > > > 306 struct device *dev = chip->gc.parent;
> > > > 307 struct attribute_group *attr_group;
> > > > 308 struct attribute **attrs;
> > > > 309 int i, ret;
> > > > 310
> > > > 311 chip->attr_groups = devm_kcalloc(dev, sizeof(*chip->attr_groups),
> > > > 312 num_lines + 1, GFP_KERNEL);
> > > > 313 if (!chip->attr_groups)
> > > > 314 return -ENOMEM;
> > > > 315
> > > > 316 for (i = 0; i < num_lines; i++) {
> > > > 317 attr_group = devm_kzalloc(dev, sizeof(*attr_group), GFP_KERNEL);
> > > > 318 attrs = devm_kcalloc(dev, GPIO_SIM_NUM_ATTRS, sizeof(*attrs),
> > > > 319 GFP_KERNEL);
> > > > 320 val_attr = devm_kzalloc(dev, sizeof(*val_attr), GFP_KERNEL);
> > > > 321 pull_attr = devm_kzalloc(dev, sizeof(*pull_attr), GFP_KERNEL);
> > > > 322 if (!attr_group || !attrs || !val_attr || !pull_attr)
> > > > 323 return -ENOMEM;
> > > > 324
> > > > 325 attr_group->name = devm_kasprintf(dev, GFP_KERNEL,
> > > > 326 "sim_gpio%u", i);
> > > > 327 if (!attr_group->name)
> > > > 328 return -ENOMEM;
> > > >
> > > > Apparently, if the memory allocation only partially succeeds, in the theoretical case
> > > > that the system is close to its kernel memory exhaustion, `return -ENOMEM` would not
> > > > free the partially succeeded allocs, would it?
> > > >
> > > > To explain it better, I tried a version that is not yet full doing "all or nothing"
> > > > memory allocation for the gpio-sim driver, because I am not that familiar with the
> > > > driver internals.
> > >
> > > devm_*() mean that the resource allocation is made in a managed manner, so when
> > > it's done, it will be freed automatically.
> >
> > Didn't see that one coming ... :-/ "buzzing though the bush ..."
> >
> > > The question is: is the lifetime of the attr_groups should be lesser or the
> > > same as chip->gc.parent? Maybe it's incorrect to call devm_*() in the first place?
> >
> > Bona fide said, I hope that automatic deallocation does things in the right order.
> > I've realised that devm_kzalloc() calls devm_kmalloc() that registers allocations on
> > a per driver list. But I am not sure how chip->gc was allocated?
> >
> > Here is said it is allocated in drivers/gpio/gpio-sim.c:386 in gpio_sim_add_bank(),
> > as a part of
> >
> > struct gpio_sim_chip *chip;
> > struct gpio_chip *gc;
> >
> > gc = &chip->gc;
> >
> > and gc->parent is set to
> >
> > gc->parent = dev;
> >
> > in line 420, which appears called before gpio_sim_setup_sysfs() and the lines above.
> >
> > If I understood well, automatic deallocation on unloading the driver goes
> > in the reverse order, so lifetime of chip appears to be longer than attr_groups,
> > but I am really not that good at this ...
>
> So, the device is instantiated by platform_device_register_full().
>
> It should gone with the platform_device_unregister().
>
> In case of CONFIG_DEBUG_KOBJECT_RELEASE=y the ->release() can be called
> asynchronously.
>
> So, there are following questions:
> - is the put_device() is actually called?
> - is the above mentioned option is set to Y?
> - if it's in Y, does kmemleak take it into account?
> - if no, do you get anything new in `dmesg` when enable it?
>
> > > Or maybe the chip->gc.parent should be changed to something else (actual GPIO
> > > device, but then it's unclear how to provide the attributes in non-racy way
> > Really, dunno. I have to repeat that my learning curve cannot adapt so quickly.
> >
> > I merely gave the report of KMEMLEAK, otherwise I am not a Linux kernel
> > device expert nor would be appropriate to try the craft not earned ;-)
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
Mirsad,
I think you fear that the memory allocated for sysfs attributes could
be accessed after the driver is detached from the simulated GPIO
device? This is not possible as sysfs handles that gracefully (by
removing all sysfs attributes with driver_sysfs_remove()) before
freeing devres resources. You can test that yourself by instantiating
a gpio-sim device, opening and holding a file descriptor to one of the
sysfs attributes, disabling the device and then trying to read from
said fd - it will return -ENODEV.
Let me know if you actually mean something else?
Bart
Powered by blists - more mailing lists