Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230802-master-v6-1-45d48299168b@kernel.org> <bac543537058619345b363bbfc745927.paul@paul-moore.com>
 <ca156cecbc070c3b7c68626572274806079a6e04.camel@kernel.org>
In-Reply-To: <ca156cecbc070c3b7c68626572274806079a6e04.camel@kernel.org>
From:   Paul Moore <paul@paul-moore.com>
Date:   Wed, 2 Aug 2023 22:46:30 -0400
Message-ID: <CAHC9VhTQDVyZewU0Oiy4AfJt_UtB7O2_-PcUmXkZtuwKDQBfXg@mail.gmail.com>
Subject: Re: [PATCH v6] vfs, security: Fix automount superblock LSM init
 problem, preventing NFS sb sharing
To:     Jeff Layton <jlayton@kernel.org>
Cc:     Alexander Viro <viro@zeniv.linux.org.uk>,
        Christian Brauner <brauner@kernel.org>,
        Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna@kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E. Hallyn" <serge@hallyn.com>,
        Stephen Smalley <stephen.smalley.work@gmail.com>,
        Eric Paris <eparis@parisplace.org>,
        Casey Schaufler <casey@schaufler-ca.com>,
        David Howells <dhowells@redhat.com>,
        Scott Mayhew <smayhew@redhat.com>,
        Stephen Smalley <sds@tycho.nsa.gov>,
        linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-nfs@vger.kernel.org, linux-security-module@vger.kernel.org,
        selinux@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Aug 2, 2023 at 3:34=E2=80=AFPM Jeff Layton <jlayton@kernel.org> wro=
te:
> On Wed, 2023-08-02 at 14:16 -0400, Paul Moore wrote:
> > On Aug  2, 2023 Jeff Layton <jlayton@kernel.org> 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=3Dsystem_u:object_r:roo=
t_t:s0
> # mount nfsserver:/export/bar /mnt/bar -o context=3Dsystem_u:object_r:roo=
t_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().

> > 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.  My only concern now is the
fs_context::lsm_set flag.

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 ...

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.

Right?

Or am I missing something really silly? :)

--=20
paul-moore.com