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;
Subject: Re: [PATCH v2 2/2] NFSv4 account for selinux security context when
 deciding to share superblock
To:     Olga Kornievskaia <olga.kornievskaia@gmail.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>,
        Casey Schaufler <casey@schaufler-ca.com>
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>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <9da50d82-85d3-3a2f-df00-586b8e3e890f@schaufler-ca.com>
Date:   Thu, 18 Feb 2021 15:17:18 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.7.1
MIME-Version: 1.0
In-Reply-To: <CAN-5tyGff24KwP-sTezU=h39_zTVTgh10D-6eT220SO=HSk3+w@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 6232
Precedence: bulk

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.


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

>  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=3D4.2,sec=3Dsys,context=3Dsystem_u:object_r:root_t:s0
>>> <serverip>:/ /mnt
>>> mount -o vers=3D4.2,sec=3Dsys,context=3Dsystem_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_c=
ontext *fc,
>>>       if (opt < 0)
>>>               return ctx->sloppy ? 1 : opt;
>>>
>>> +     if (fc->security)
>>> +             ctx->has_sec_mnt_opts =3D 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 =3D ctx->has_sec_mnt_opts;
>>>  }
>>>
>>>  static int nfs_compare_mount_options(const struct super_block *s, co=
nst 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 */