selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Al Viro <viro@ZenIV.linux.org.uk>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: selinux@tycho.nsa.gov, Paul Moore <paul@paul-moore.com>,
	linux-fsdevel@vger.kernel.org, stable@vger.kernel.org,
	Stephen Smalley <sds@tycho.nsa.gov>
Subject: Re: [PATCH] selinux: fix race when removing selinuxfs entries
Date: Tue, 2 Oct 2018 16:58:10 +0100	[thread overview]
Message-ID: <20181002155810.GP32577@ZenIV.linux.org.uk> (raw)
In-Reply-To: <20181002111830.26342-1-omosnace@redhat.com>

On Tue, Oct 02, 2018 at 01:18:30PM +0200, Ondrej Mosnacek wrote:

No.  With the side of Hell, No.  The bug is real, but this is
not the way to fix it.

First of all, it's still broken - e.g. mount something on a
subdirectory and watch what that thing will do to it.  And
anyone who has permission to reload policy _will_ have one
for mount.

And yes, debugfs_remove() suffers the same problem.  Frankly, the
only wish debugfs_remove() implementation inspires is to shoot it
before it breeds.  Which is the second problem with that approach -
such stuff really shouldn't be duplicated in individual filesystems.  

Don't copy (let along open-code) d_walk() in there.  The guts of
dcache tree handling are subtle enough (and had more than enough
locking bugs over the years) to make spreading the dependencies
on its details all over the tree an invitation for massive PITA
down the road.

I have beginnings of patch doing that for debugfs; the same thing
should be usable for selinuxfs as well.  However, the main problem
with selinuxfs wrt policy loading is different - what to do if
sel_make_policy_nodes() fails halfway through?  And what to do
with accesses to the unholy mix of old and new nodes we currently
have left behind?

Before security_load_policy() we don't know which nodes to create;
after it we have nothing to fall back onto.  It looks like we
need to split security_load_policy() into "load", "switch over" and
"free old" parts, so that the whole thing would look like
	load
	create new directory trees, unconnected to the root so
they couldn't be reached by lookups until we are done
	switch over
	move the new directory trees in place
	kill the old trees (using that invalidate+genocide carefully
combination)
	free old data structures
with failures upon the first two steps handled by killing whatever detached
trees we'd created (if any) and freeing the new data structures.

However, I'd really like to have the folks familiar with selinux guts to
comment upon the feasibility of the above.  AFAICS, nobody has ever seriously
looked at that code wrt graceful error handling, etc.[*], so I'm not happy
with making inferences from what the existing code is doing.

If you are interested in getting selinuxfs into sane shape, that would
be a good place to start.  As for the kernel-side rm -rf (which is what
debugfs_remove() et.al. are trying to be)...
	* it absolutely needs to be in fs/*.c - either dcache or libfs.
It's too closely tied to dcache guts to do otherwise.
	* as the first step it needs to do d_invalidate(), to get rid of
anything that might be mounted on it and to prevent new mounts from appearing.
It's rather tempting to unhash everything in the victim tree at the same
time, but that needs to be done with care - I'm still not entirely happy
with the solution I've got in that area.  Alternative is to unhash them
on the way out of subtree.  simple_unlink()/simple_rmdir() are wrong
there - we don't want to bother with the parent's timestamps as we go,
for one thing; that should be done only once to parent of the root of
that subtree.  For another, we bloody well enforce the emptiness ourselves,
so this simple_empty() is pointless (especially since we have no choice other
than ignoring it anyway).

BTW, another selinuxfs unpleasantness is, the things like sel_write_enforce()
don't have any exclusion against themselves, let alone the policy reloads.
And quite a few of them obviously expect that e.g. permission check is done
against the same policy the operation will apply to, not the previous one.
That one definitely needs selinux folks involved.

[*] not too unreasonably so - anyone who gets to use _that_ as an attack
vector has already won, so it's not a security problem pretty much by
definition and running into heavy OOM at the time of policy reload is
almost certainly going to end up with the userland parts of the entire
thing not handling failures gracefully.

  reply	other threads:[~2018-10-02 15:58 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-02 11:18 Ondrej Mosnacek
2018-10-02 15:58 ` Al Viro [this message]
2018-10-03  8:18   ` Ondrej Mosnacek
2018-10-03 13:02   ` Stephen Smalley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181002155810.GP32577@ZenIV.linux.org.uk \
    --to=viro@zeniv.linux.org.uk \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=omosnace@redhat.com \
    --cc=paul@paul-moore.com \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@tycho.nsa.gov \
    --cc=stable@vger.kernel.org \
    --subject='Re: [PATCH] selinux: fix race when removing selinuxfs entries' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).