From: Stephen Smalley <sds@tycho.nsa.gov>
To: Al Viro <viro@ZenIV.linux.org.uk>, 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
Subject: Re: [PATCH] selinux: fix race when removing selinuxfs entries
Date: Wed, 3 Oct 2018 09:02:29 -0400 [thread overview]
Message-ID: <47eb1f5b-71f7-3407-c381-5c091bdabfdc@tycho.nsa.gov> (raw)
In-Reply-To: <20181002155810.GP32577@ZenIV.linux.org.uk>
On 10/02/2018 11:58 AM, Al Viro wrote:
> 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.
That sounds viable to me as an approach, and hopefully the
splitting/refactoring of security_load_policy() will be a bit easier now
that the global selinux state is wrapped in the selinux_state structure
and explicitly passed around.
>
> 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.
>
prev parent reply other threads:[~2018-10-03 13:02 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-02 11:18 [PATCH] selinux: fix race when removing selinuxfs entries Ondrej Mosnacek
2018-10-02 15:58 ` Al Viro
2018-10-03 8:18 ` Ondrej Mosnacek
2018-10-03 13:02 ` Stephen Smalley [this message]
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=47eb1f5b-71f7-3407-c381-5c091bdabfdc@tycho.nsa.gov \
--to=sds@tycho.nsa.gov \
--cc=linux-fsdevel@vger.kernel.org \
--cc=omosnace@redhat.com \
--cc=paul@paul-moore.com \
--cc=selinux@tycho.nsa.gov \
--cc=stable@vger.kernel.org \
--cc=viro@ZenIV.linux.org.uk \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).