Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210218195046.19280-1-olga.kornievskaia@gmail.com>
 <20210218195046.19280-2-olga.kornievskaia@gmail.com> <7587df15-6f6f-3581-8bae-a648315cfae9@schaufler-ca.com>
 <CAN-5tyGff24KwP-sTezU=h39_zTVTgh10D-6eT220SO=HSk3+w@mail.gmail.com> <9da50d82-85d3-3a2f-df00-586b8e3e890f@schaufler-ca.com>
In-Reply-To: <9da50d82-85d3-3a2f-df00-586b8e3e890f@schaufler-ca.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Fri, 19 Feb 2021 12:11:10 -0500
Message-ID: <CAN-5tyEkaiePjMhssvOw_JbAB57n0d_QZEgJwHk3362eJjdM8w@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] NFSv4 account for selinux security context when
 deciding to share superblock
To:     Casey Schaufler <casey@schaufler-ca.com>
Cc:     Trond Myklebust <trond.myklebust@hammerspace.com>,
        Anna Schumaker <anna.schumaker@netapp.com>,
        linux-nfs <linux-nfs@vger.kernel.org>,
        Linux Security Module list 
        <linux-security-module@vger.kernel.org>,
        SElinux list <selinux@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 18, 2021 at 6:17 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>
> On 2/18/2021 2:39 PM, Olga Kornievskaia wrote:
> > On Thu, Feb 18, 2021 at 5:07 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> >> On 2/18/2021 11:50 AM, Olga Kornievskaia wrote:
> >>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>
> >>> Keep track of whether or not there was an selinux context mount
> >>> options during the mount.
> >> This may be the intention, but it's not what the change you're
> >> introducing here does.
> > Can you explain why, as I must be doing something wrong? I'm familiar
> > with NFS but not with Selinux and I thought I was doing the correct
> > changes to the Selinux. If that's not the case would you share what
> > has been done incorrectly.
>
> The code in this patch is generic for any LSM that wants to provide
> a security_sb_mnt_opts_compat() hook. Your 1/2 patch deals with how
> the hook provided by SELinux behaves.  All the behavior that is specific
> to SELinux should be in the SELinux hook. If you can state the behavior
> generically you should do that here.

Hopefully by removing specific reference to the selinux and sticking
with LSM addresses your comment. To NFS it's a security context it
doesn't care if it's selinux or something else.

> >>>  While deciding if the superblock can be
> >>> shared for the new mount, check for if we had selinux context on
> >>> the existing mount and call into selinux to tell if new passed
> >>> in selinux context is compatible with the existing mount's options.
> >> You're describing how you expect the change to be used, not
> >> what it does. If I am the author of a security module other
> >> than SELinux (which, it turns out, I am) what would I use this
> >> for? How might this change interact with my security module?
> >> Is this something I might exploit? If I am the author of a
> >> filesystem other than NFS (which I am not) should I be doing
> >> the same thing?
> > I'm not sure I'm understanding your questions. I'm solving a bug that
> > exists when NFS interacts with selinux.
>
> Right, but you're introducing an LSM interface to do so. LSM interfaces
> are expected to be usable by any security module. Smack ought to be able
> to provide NFS support, so might be expected to provide a hook for
> security_sb_mnt_opts_compat().

So I thought to address how a filesystem uses the hooks should have
been in the first patch and I added it here. But addressing how
another LSM module (like Smack) would use it again should be coming
from the first patch. It's a new hook to compare mount options and if
Smack were to implement some security mount options it would implement
a comparison function of the two.


>
> >  This is really not a feature
> > that I'm introducing. I thought it was somewhat descriptive with an
> > example that if you want to mount with different security contexts but
> > whatever you are mounting has a possibility of sharing the same
> > superblock, we need to be careful and take security contexts that are
> > being used by those mount into account to decide whether or not to
> > share the superblock. Again I thought that's exactly what the commit
> > states. A different security module, if it acts on security context,
> > would have to have the same ability to compare if one context options
> > are compatible with anothers.
>
> Not everyone is going to extrapolate the general behavior from
> the SELinux behavior. Your description of the SELinux behavior in 1/2
> is fine. I'm looking for how a module other than SELinux would use
> this.
>
> >  Another filesystem can decide if it
> > wants to share superblocks based on compatibility of existing security
> > context and new one. Is that what you are asking?
>
> What I'm asking is whether this should be a concern for filesystems
> in general, in which case this isn't the right place to make this change.
>
> >
> >
> >>> Previously, NFS wasn't able to do the following 2mounts:
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:root_t:s0
> >>> <serverip>:/ /mnt
> >>> mount -o vers=4.2,sec=sys,context=system_u:object_r:swapfile_t:s0
> >>> <serverip>:/scratch /scratch
> >>>
> >>> 2nd mount would fail with "mount.nfs: an incorrect mount option was
> >>> specified" and var log messages would have:
> >>> "SElinux: mount invalid. Same superblock, different security
> >>> settings for.."
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>> ---
> >>>  fs/nfs/fs_context.c       | 3 +++
> >>>  fs/nfs/internal.h         | 1 +
> >>>  fs/nfs/super.c            | 4 ++++
> >>>  include/linux/nfs_fs_sb.h | 1 +
> >>>  4 files changed, 9 insertions(+)
> >>>
> >>> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> >>> index 06894bcdea2d..8067f055d842 100644
> >>> --- a/fs/nfs/fs_context.c
> >>> +++ b/fs/nfs/fs_context.c
> >>> @@ -448,6 +448,9 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
> >>>       if (opt < 0)
> >>>               return ctx->sloppy ? 1 : opt;
> >>>
> >>> +     if (fc->security)
> >>> +             ctx->has_sec_mnt_opts = 1;
> >>> +
> >>>       switch (opt) {
> >>>       case Opt_source:
> >>>               if (fc->source)
> >>> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> >>> index 62d3189745cd..08f4f34e8cf5 100644
> >>> --- a/fs/nfs/internal.h
> >>> +++ b/fs/nfs/internal.h
> >>> @@ -96,6 +96,7 @@ struct nfs_fs_context {
> >>>       char                    *fscache_uniq;
> >>>       unsigned short          protofamily;
> >>>       unsigned short          mountfamily;
> >>> +     bool                    has_sec_mnt_opts;
> >>>
> >>>       struct {
> >>>               union {
> >>> diff --git a/fs/nfs/super.c b/fs/nfs/super.c
> >>> index 4034102010f0..0a2d252cf90f 100644
> >>> --- a/fs/nfs/super.c
> >>> +++ b/fs/nfs/super.c
> >>> @@ -1058,6 +1058,7 @@ static void nfs_fill_super(struct super_block *sb, struct nfs_fs_context *ctx)
> >>>                                                &sb->s_blocksize_bits);
> >>>
> >>>       nfs_super_set_maxbytes(sb, server->maxfilesize);
> >>> +     server->has_sec_mnt_opts = ctx->has_sec_mnt_opts;
> >>>  }
> >>>
> >>>  static int nfs_compare_mount_options(const struct super_block *s, const struct nfs_server *b,
> >>> @@ -1174,6 +1175,9 @@ static int nfs_compare_super(struct super_block *sb, struct fs_context *fc)
> >>>               return 0;
> >>>       if (!nfs_compare_userns(old, server))
> >>>               return 0;
> >>> +     if ((old->has_sec_mnt_opts || fc->security) &&
> >>> +                     security_sb_mnt_opts_compat(sb, fc->security))
> >>> +             return 0;
> >>>       return nfs_compare_mount_options(sb, server, fc);
> >>>  }
> >>>
> >>> diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> >>> index 38e60ec742df..3f0acada5794 100644
> >>> --- a/include/linux/nfs_fs_sb.h
> >>> +++ b/include/linux/nfs_fs_sb.h
> >>> @@ -254,6 +254,7 @@ struct nfs_server {
> >>>
> >>>       /* User namespace info */
> >>>       const struct cred       *cred;
> >>> +     bool                    has_sec_mnt_opts;
> >>>  };
> >>>
> >>>  /* Server capabilities */
>