On Mon, Oct 16, 2023 at 6:08 PM Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > > [ > That thing sits in viro/vfs.git#work.selinuxfs; I have > lock_rename()-related followups in another branch, so a pull would be more > convenient for me than cherry-pick. NOTE: testing and comments would > be very welcome - as it is, the patch is pretty much untested beyond > "it builds". > ] Hi Al, I will admit to glossing over the comment above when I merged this into the selinux/dev branch last night. As it's been a few weeks, I'm not sure if the comment above still applies, but if it does let me know and I can yank/revert the patch in favor of a larger pull. Let me know what you'd like to do. > On policy reload selinuxfs replaces two subdirectories (/booleans > and /class) with new variants. Unfortunately, that's done with > serious abuses of directory locking. > > 1) lock_rename() should be done to parents, not to objects being > exchanged > > 2) there's a bunch of reasons why it should not be done for directories > that do not have a common ancestor; most of those do not apply to > selinuxfs, but even in the best case the proof is subtle and brittle. > > 3) failure halfway through the creation of /class will leak > names and values arrays. > > 4) use of d_genocide() is also rather brittle; it's probably not much of > a bug per se, but e.g. an overmount of /sys/fs/selinuxfs/classes/shm/index > with any regular file will end up with leaked mount on policy reload. > Sure, don't do it, but... > > Let's stop messing with disconnected directories; just create > a temporary (/.swapover) with no permissions for anyone (on the > level of ->permission() returing -EPERM, no matter who's calling > it) and build the new /booleans and /class in there; then > lock_rename on root and that temporary directory and d_exchange() > old and new both for class and booleans. Then unlock and use > simple_recursive_removal() to take the temporary out; it's much > more robust. > > And instead of bothering with separate pathways for freeing > new (on failure halfway through) and old (on success) names/values, > do all freeing in one place. With temporaries swapped with the > old ones when we are past all possible failures. > > The only user-visible difference is that /.swapover shows up > (but isn't possible to open, look up into, etc.) for the > duration of policy reload. > > Signed-off-by: Al Viro <viro@xxxxxxxxxxxxxxxxxx> > --- > diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c -- paul-moore.com
