linux-kernel - Re: [GIT PULL] gpio: updates for v5.13

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 5 May 2021 16:19:52 +0200
From:   Bartosz Golaszewski <brgl@...ev.pl>
To:     Al Viro <viro@...iv.linux.org.uk>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [GIT PULL] gpio: updates for v5.13

On Tue, May 4, 2021 at 7:34 PM Al Viro <viro@...iv.linux.org.uk> wrote:
>
> On Tue, May 04, 2021 at 04:17:02PM +0200, Bartosz Golaszewski wrote:
> > >     Incidentally, if your code critically depends upon some field
> > > being first in such-and-such structure, you should either get rid of
> > > the dependency or at least bother to document that.
> > > That
> > > +               /*
> > > +                * Free memory allocated for the pending and live
> > > directories
> > > +                * of committable groups.
> > > +                */
> > > +               if (sd->s_type & (CONFIGFS_GROUP_PENDING |
> > > CONFIGFS_GROUP_LIVE))
> > > +                       kfree(sd->s_element);
> > > +
> > > is asking for trouble down the road.
> > >
> >
> > I'm not sure if this is a hard NAK for these changes or if you
> > consider this something that can be ironed out post v5.13-rc1?
>
> Rename implementation is simply bogus.  You are, for some reason, attaching
> stuff to *destination*, which won't be seen by anyone not currently using
> it.  It's the old_dentry that will be seen from that point on - you are
> moving it to new location by that d_move().  So I rather wonder how much
> had that thing been tested.  And I'm pretty much certain that you are
> mishandling the refcounts on configfs-internal objects, with everything
> that entails in terms of UAF and leaks.
>

The interface's stability in user-space has been tested a lot with the
test-suite for libgpiod[1] but I didn't look for leaks indeed.

> FWIW, I'm not happy about the userland API of that thing (what is supposed
> to happen if you create, move to live, then create another with the same
> name and try to move it to live or original back from live?), but
> Documentation/filesystems/configfs.rst is too sparse on such details.
> So I would like to see the specifics on that as well.  _Before_ signing
> up on anything, including "we can fix it up after merge".

Understood. I've sent out a new PR without these changes. I'll start
another thread asking for your help on the correct approach and maybe
some better ideas for the user interface.

Thanks,
Bartosz

[1] https://patchwork.ozlabs.org/project/linux-gpio/patch/20210429094734.9585-3-brgl@bgdev.pl/