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:   Tue, 4 May 2021 01:55:42 +0000
From:   Al Viro <viro@...iv.linux.org.uk>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Bartosz Golaszewski <brgl@...ev.pl>,
        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 Mon, May 03, 2021 at 06:28:38PM +0000, Al Viro wrote:

> > So Al, do you see anything horrendous in how that configfs thing uses
> > a rename to do kind of an "atomic swap" of configfs state?
> 
> Give me a few hours; configfs is playing silly buggers with a lot of
> structures when creating/tearing down subtrees, and I'd actually
> expect more trouble with configfs data structures than with VFS ones.
> 
> I'll take a look.

FWIW, one obviously bogus thing is this:

+       spin_lock(&configfs_dirent_lock);
+       new_dentry->d_fsdata = sd;
+       list_move(&sd->s_sibling, &new_parent_sd->s_children);
+       item->ci_parent = new_parent_item;
+       d_move(old_dentry, new_dentry);
+       spin_unlock(&configfs_dirent_lock);
on successful ->rename().  sd here comes from
+       sd = old_dentry->d_fsdata;

	Now, take a look at configfs_d_iput().  ->d_fsdata contributes
to refcount of sd, and I don't see anything here that would grab the
reference.

	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 dislike (for the lack of adequate printable terms) the way configfs 
deals with subtree creation and, especially, removal.  It's kept attached
to dentry tree (all the way to the root) as we build it and, in case we
fail halfway through, as we are trying to take it apart.

	There is convoluted code trying to prevent breakage in such cases,
but it's complex, brittle and I don't remember how critical the lack of
renames had been in its analysis.  I can try to redo that, but that would
take some time - IIRC, the last time I did it, it took several days
of work (including arseloads of grepping through configfs users and
doing RTFS in those)

	IMO we should attach the subtree we'd built only when it's
fully set up.  I can dig out the notes (from 2 years ago) on how to massage
the damn thing in that direction, but again, it'll take a day or two
to verify that analysis still applies.  OTOH, that would simplify the code
considerably, so the next time we want to change something it wouldn't
be so unpleasant.