On Wed, 2023-08-02 at 14:16 -0400, Paul Moore wrote:
> On Aug 2, 2023 Jeff Layton <jlayton@xxxxxxxxxx> wrote:
> >
> > When NFS superblocks are created by automounting, their LSM parameters
> > aren't set in the fs_context struct prior to sget_fc() being called,
> > leading to failure to match existing superblocks.
> >
> > Fix this by adding a new LSM hook to load fc->security for submount
> > creation when alloc_fs_context() is creating the fs_context for it.
> >
> > However, this uncovers a further bug: nfs_get_root() initialises the
> > superblock security manually by calling security_sb_set_mnt_opts() or
> > security_sb_clone_mnt_opts() - but then vfs_get_tree() calls
> > security_sb_set_mnt_opts(), which can lead to SELinux, at least,
> > complaining.
> >
> > Fix that by adding a flag to the fs_context that suppresses the
> > security_sb_set_mnt_opts() call in vfs_get_tree(). This can be set by NFS
> > when it sets the LSM context on the new superblock.
> >
> > The first bug leads to messages like the following appearing in dmesg:
> >
> > NFS: Cache volume key already in use (nfs,4.2,2,108,106a8c0,1,,,,100000,100000,2ee,3a98,1d4c,3a98,1)
> >
> > Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Fixes: 9bc61ab18b1d ("vfs: Introduce fs_context, switch vfs_kern_mount() to it.")
> > Fixes: 779df6a5480f ("NFS: Ensure security label is set for root inode)
> > Tested-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > Acked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
> > Acked-by: "Christian Brauner (Microsoft)" <brauner@xxxxxxxxxx>
> > Link: https://lore.kernel.org/r/165962680944.3334508.6610023900349142034.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v1
> > Link: https://lore.kernel.org/r/165962729225.3357250.14350728846471527137.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v2
> > Link: https://lore.kernel.org/r/165970659095.2812394.6868894171102318796.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v3
> > Link: https://lore.kernel.org/r/166133579016.3678898.6283195019480567275.stgit@xxxxxxxxxxxxxxxxxxxxxx/ # v4
> > Link: https://lore.kernel.org/r/217595.1662033775@xxxxxxxxxxxxxxxxxxxxxx/ # v5
> > ---
> > This patch was originally sent by David several months ago, but it
> > never got merged. I'm resending to resurrect the discussion. Can we
> > get this fixed?
>
> Sorry, I sorta lost track of this after the ROOTCONTEXT_MNT discussion
> back in v3. Looking at it a bit closer now I have one nitpicky
> request and one larger concern (see below).
>
> > diff --git a/fs/super.c b/fs/super.c
> > index e781226e2880..13adf43e2e5d 100644
> > --- a/fs/super.c
> > +++ b/fs/super.c
> > @@ -1541,10 +1541,12 @@ int vfs_get_tree(struct fs_context *fc)
> > smp_wmb();
> > sb->s_flags |= SB_BORN;
> >
> > - error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL);
> > - if (unlikely(error)) {
> > - fc_drop_locked(fc);
> > - return error;
> > + if (!(fc->lsm_set)) {
> > + error = security_sb_set_mnt_opts(sb, fc->security, 0, NULL);
> > + if (unlikely(error)) {
> > + fc_drop_locked(fc);
> > + return error;
> > + }
> > }
>
> 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).
The problems that David hit with the automounter have a similar root
cause though, I believe.
> 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.
> In a worst case situation, we could always implement a flag *inside*
> the SELinux code, similar to what has been done with 'lsm_set' here.
>
I'm fine with a different solution, if you see a better one. You'll have
to handhold me through this one though. LSM stuff is not really my
forte'. Let me know what you'd like to see here.
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index d06e350fedee..29cce0fadbeb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -2745,6 +2745,30 @@ static int selinux_umount(struct vfsmount *mnt, int flags)
> > FILESYSTEM__UNMOUNT, NULL);
> > }
> >
> > +static int selinux_fs_context_init(struct fs_context *fc,
> > + struct dentry *reference)
> > +{
> > + const struct superblock_security_struct *sbsec;
> > + struct selinux_mnt_opts *opts;
> > +
> > + if (fc->purpose == FS_CONTEXT_FOR_SUBMOUNT) {
> > + opts = kzalloc(sizeof(*opts), GFP_KERNEL);
> > + if (!opts)
> > + return -ENOMEM;
> > +
> > + sbsec = selinux_superblock(reference->d_sb);
> > + if (sbsec->flags & FSCONTEXT_MNT)
> > + opts->fscontext_sid = sbsec->sid;
> > + if (sbsec->flags & CONTEXT_MNT)
> > + opts->context_sid = sbsec->mntpoint_sid;
> > + if (sbsec->flags & DEFCONTEXT_MNT)
> > + opts->defcontext_sid = sbsec->def_sid;
>
> I acknowledge this is very nitpicky, but we're starting to make a
> greater effort towards using consistent style within the SELinux
> code. With that in mind, please remove the alignment whitespace in
> the assignments above. Thank you.
>
Will do. Thanks for having a look!
> > + fc->security = opts;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int selinux_fs_context_dup(struct fs_context *fc,
> > struct fs_context *src_fc)
> > {
>
> --
> paul-moore.com
--
Jeff Layton <jlayton@xxxxxxxxxx>
