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

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

Message-ID: <YJGFsrPBoQsKj+JZ@zeniv-ca.linux.org.uk>
Date:   Tue, 4 May 2021 17:34:42 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Bartosz Golaszewski <brgl@...ev.pl>
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 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.

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".