linux-kernel - Re: [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for setting lineinfo

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Tue, 17 Mar 2020 14:40:51 +0100
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Geert Uytterhoeven <geert@...ux-m68k.org>,
        Linus Walleij <linus.walleij@...aro.org>
Cc:     Kent Gibson <warthog618@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@...libre.com>
Subject: Re: [RESEND PATCH v6 5/7] gpiolib: provide a dedicated function for
 setting lineinfo

pon., 16 mar 2020 o 17:21 Geert Uytterhoeven <geert@...ux-m68k.org> napisał(a):
>
> Hi Bartosz,
>
> On Tue, Feb 11, 2020 at 10:21 AM Bartosz Golaszewski <brgl@...ev.pl> wrote:
> > From: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > We'll soon be filling out the gpioline_info structure in multiple
> > places. Add a separate function that given a gpio_desc sets all relevant
> > fields.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@...libre.com>
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>
> This is now commit d2ac25798208fb85 ("gpiolib: provide a dedicated
> function for setting lineinfo") in gpio/for-next.
>
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1147,6 +1147,60 @@ static int lineevent_create(struct gpio_device *gdev, void __user *ip)
> >         return ret;
> >  }
> >
> > +static void gpio_desc_to_lineinfo(struct gpio_desc *desc,
> > +                                 struct gpioline_info *info)
> > +{
> > +       struct gpio_chip *chip = desc->gdev->chip;
> > +       unsigned long flags;
> > +
> > +       spin_lock_irqsave(&gpio_lock, flags);
>
> spinlock taken
>
> > +
> > +       if (desc->name) {
> > +               strncpy(info->name, desc->name, sizeof(info->name));
> > +               info->name[sizeof(info->name) - 1] = '\0';
> > +       } else {
> > +               info->name[0] = '\0';
> > +       }
> > +
> > +       if (desc->label) {
> > +               strncpy(info->consumer, desc->label, sizeof(info->consumer));
> > +               info->consumer[sizeof(info->consumer) - 1] = '\0';
> > +       } else {
> > +               info->consumer[0] = '\0';
> > +       }
> > +
> > +       /*
> > +        * Userspace only need to know that the kernel is using this GPIO so
> > +        * it can't use it.
> > +        */
> > +       info->flags = 0;
> > +       if (test_bit(FLAG_REQUESTED, &desc->flags) ||
> > +           test_bit(FLAG_IS_HOGGED, &desc->flags) ||
> > +           test_bit(FLAG_USED_AS_IRQ, &desc->flags) ||
> > +           test_bit(FLAG_EXPORT, &desc->flags) ||
> > +           test_bit(FLAG_SYSFS, &desc->flags) ||
> > +           !pinctrl_gpio_can_use_line(chip->base + info->line_offset))
>
> pinctrl_gpio_can_use_line(), and pinctrl_get_device_gpio_range() called
> from it, call mutex_lock():
>
>     BUG: sleeping function called from invalid context at
> kernel/locking/mutex.c:281
>     in_atomic(): 1, irqs_disabled(): 128, non_block: 0, pid: 652, name: lsgpio
>     CPU: 1 PID: 652 Comm: lsgpio Not tainted
> 5.6.0-rc1-koelsch-00008-gd2ac25798208fb85 #755
>     Hardware name: Generic R-Car Gen2 (Flattened Device Tree)
>     [<c020e3f0>] (unwind_backtrace) from [<c020a5b8>] (show_stack+0x10/0x14)
>     [<c020a5b8>] (show_stack) from [<c07d31b4>] (dump_stack+0x88/0xa8)
>     [<c07d31b4>] (dump_stack) from [<c0241318>] (___might_sleep+0xf8/0x168)
>     [<c0241318>] (___might_sleep) from [<c07ec13c>] (mutex_lock+0x24/0x7c)
>     [<c07ec13c>] (mutex_lock) from [<c046f47c>]
> (pinctrl_get_device_gpio_range+0x1c/0xb4)
>     [<c046f47c>] (pinctrl_get_device_gpio_range) from [<c046f5e8>]
> (pinctrl_gpio_can_use_line+0x24/0x88)
>     [<c046f5e8>] (pinctrl_gpio_can_use_line) from [<c0478bd0>]
> (gpio_ioctl+0x270/0x584)
>     [<c0478bd0>] (gpio_ioctl) from [<c03194c0>] (vfs_ioctl+0x20/0x38)
>
> Reproducer is "lsgpio" with CONFIG_DEBUG_ATOMIC_SLEEP=y.
>

Hi Geert,

thanks for the report. I added the locking around this code because it
seemed wrong to me that this part wasn't protected in any way before.
Now the question is how do we deal with this.

Linus: do you think it's safe to move the call to
pinctrl_gpio_can_use_line() before the spinlock is taken? I'd say yes
but I'm not sure if I'm not missing some inter-framework intricacies.

Best regards,
Bartosz Golaszewski