Re: [PATCH v6] vfs, security: Fix automount superblock LSM init problem, preventing NFS sb sharing

[Jeff Layton]

On Wed, 2023-08-02 at 22:46 -0400, Paul Moore wrote:
> On Wed, Aug 2, 2023 at 3:34 PM Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> > On Wed, 2023-08-02 at 14:16 -0400, Paul Moore wrote:
> > > On Aug  2, 2023 Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> 
> ...
> 
> > > I generally dislike core kernel code which makes LSM calls conditional
> > > on some kernel state maintained outside the LSM.  Sometimes it has to
> > > be done as there is no other good options, but I would like us to try
> > > and avoid it if possible.  The commit description mentioned that this
> > > was put here to avoid a SELinux complaint, can you provide an example
> > > of the complain?  Does it complain about a double/invalid mount, e.g.
> > > "SELinux: mount invalid.  Same superblock, different security ..."?
> > 
> > The problem I had was not so much SELinux warnings, but rather that in a
> > situation where I would expect to share superblocks between two
> > filesystems, it didn't.
> > 
> > Basically if you do something like this:
> > 
> > # mount nfsserver:/export/foo /mnt/foo -o context=system_u:object_r:root_t:s0
> > # mount nfsserver:/export/bar /mnt/bar -o context=system_u:object_r:root_t:s0
> > 
> > ...when "foo" and "bar" are directories on the same filesystem on the
> > server, you should get two vfsmounts that share a superblock. That's
> > what you get if selinux is disabled, but not when it's enabled (even
> > when it's in permissive mode).
> 
> Thanks, that helps.  I'm guessing the difference in behavior is due to
> the old->has_sec_mnt_opts check in nfs_compare_super().
> 

Yep. That gets set, but fc->security is still NULL.

> > > I'd like to understand why the sb_set_mnt_opts() call fails when it
> > > comes after the fs_context_init() call.  I'm particulary curious to
> > > know if the failure is due to conflicting SELinux state in the
> > > fs_context, or if it is simply an issue of sb_set_mnt_opts() not
> > > properly handling existing values.  Perhaps I'm being overly naive,
> > > but I'm hopeful that we can address both of these within the SELinux
> > > code itself.
> > 
> > The problem I hit was that nfs_compare_super is called with a fs_context
> > that has a NULL ->security pointer. That caused it to call
> > selinux_sb_mnt_opts_compat with mnt_opts set to NULL, and at that point
> > it returns 1 and decides not to share sb's.
> > 
> > Filling out fc->security with this new operation seems to fix that, but
> > if you see a better way to do this, then I'm certainly open to the idea.
> 
> Just as you mention that you are not a LSM expert, I am not a VFS
> expert, so I think we'll have to help each other a bit ;)
> 
> I think I'm beginning to understand alloc_fs_context() a bit more,
> including the fs_context_for_XXX() wrappers.  One thing I have
> realized is that I believe we need to update the
> selinux_fs_context_init() and smack_fs_context_init() functions to
> properly handle a NULL @reference dentry; I think returning without
> error in both cases is the correct answer.  In the non-NULL @reference
> case, I believe your patch is correct, we do want to inherit the
> options from @reference.
> 


ACK. That seems reasonable. I'll work that in.


>   My only concern now is the
> fs_context::lsm_set flag.
> 

Yeah, that bit is ugly. David studied this problem a lot more than I
have, but basically, we only want to set the context info once, and
we're not always going to have a nice string to parse to set up the
options. This obviously works, but I'm fine with a more elegant method
if you can spot one.


> You didn't mention exactly why the security_sb_set_mnt_opts() was
> failing, and requires the fs_context::lsm_set check, but my guess is
> that something is tripping over the fact that the superblock is
> already properly setup.  I'm working under the assumption that this
> problem - attempting to reconfigure a properly configured superblock -
> should only be happening in the submount/non-NULL-reference case.  If
> it is happening elsewhere I think I'm going to need some help
> understanding that ...
> 

Correct. When you pass in the mount options, fc->security seems to be
properly set. NFS mounting is complex though, so the final superblock
you care about may end up being a descendant of the one that was
originally configured.

This patch is intended to ensure we carry over security info in these
cases. We already try to inherit other parameters from parent mounts, so
this is just another set that we need to make sure we inherit.

> However, assuming I'm mostly correct in the above paragraph, would it
> be possible to take a reference to the @reference dentry's superblock
> in security_fs_context_init(), that we could later compare to the
> superblock passed into security_sb_set_mnt_opts()?  If we know that
> the fs_context was initialized with the same superblock we are now
> being asked to set mount options on, we should be able to return from
> the LSM hook without doing anything.
> 

I'm not sure that I follow your logic here:

You want to take a sb reference and carry that in the fs_context? What
will you do with it in security_sb_set_mnt_opts?

FWIW, It's generally easier to deal with inode or dentry references than
refs to the superblock too, so if we want to carry a reference to an
object around, we'd probably rather handle one of those.
-- 
Jeff Layton <jlayton@xxxxxxxxxx>