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 1/2] [security] Add new hook to compare new mount to an
 existing mount
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: <20210212211955.11239-1-olga.kornievskaia@gmail.com>
 <6214d043-3250-43bc-d668-bc9ffa8c9af2@schaufler-ca.com>
 <CAN-5tyFU7JCKEr6wPQ3_LTGJHQLFmrb=-xWQt6mGhv7JzE_xXQ@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
Message-ID: <7f2c84d8-6bef-b895-2e08-c0579d6faad9@schaufler-ca.com>
Date:   Fri, 12 Feb 2021 16:32:46 -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-5tyFU7JCKEr6wPQ3_LTGJHQLFmrb=-xWQt6mGhv7JzE_xXQ@mail.gmail.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Content-Language: en-US
Content-Length: 8766
Precedence: bulk

On 2/12/2021 2:50 PM, Olga Kornievskaia wrote:
> Thank you for comment Casey. Comments are in line.
>
> On Fri, Feb 12, 2021 at 5:37 PM Casey Schaufler <casey@schaufler-ca.com=
> wrote:
>> On 2/12/2021 1:19 PM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <kolga@netapp.com>
>>>
>>> Add a new hook that takes an existing super block and a new mount
>>> with new options and determines if new options confict with an
>>> existing mount or not.
>>>
>>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
>>> ---
>>>  include/linux/lsm_hook_defs.h |  1 +
>>>  include/linux/lsm_hooks.h     |  6 ++++
>>>  include/linux/security.h      |  1 +
>>>  security/security.c           |  7 +++++
>>>  security/selinux/hooks.c      | 54 +++++++++++++++++++++++++++++++++=
++
>>>  5 files changed, 69 insertions(+)
>>>
>>> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_d=
efs.h
>>> index 7aaa753b8608..fbfc07d0b3d5 100644
>>> --- a/include/linux/lsm_hook_defs.h
>>> +++ b/include/linux/lsm_hook_defs.h
>>> @@ -62,6 +62,7 @@ LSM_HOOK(int, 0, sb_alloc_security, struct super_bl=
ock *sb)
>>>  LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *s=
b)
>>>  LSM_HOOK(void, LSM_RET_VOID, sb_free_mnt_opts, void *mnt_opts)
>>>  LSM_HOOK(int, 0, sb_eat_lsm_opts, char *orig, void **mnt_opts)
>>> +LSM_HOOK(int, 0, sb_do_mnt_opts_match, struct super_block *sb, void =
*mnt_opts)
>> Don't you want this to be sb_mnt_opts_compatible ?
>> They may not have to match. They just need to be acceptable
>> to the security module. SELinux doesn't appear to require
>> that they "match" in all respects.
> You are right, it was a poor naming choice. Compatible is better. I
> can change it.
>>
>>>  LSM_HOOK(int, 0, sb_remount, struct super_block *sb, void *mnt_opts)=

>>>  LSM_HOOK(int, 0, sb_kern_mount, struct super_block *sb)
>>>  LSM_HOOK(int, 0, sb_show_options, struct seq_file *m, struct super_b=
lock *sb)
>>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
>>> index a19adef1f088..a11b062c1847 100644
>>> --- a/include/linux/lsm_hooks.h
>>> +++ b/include/linux/lsm_hooks.h
>>> @@ -142,6 +142,12 @@
>>>   *   @orig the original mount data copied from userspace.
>>>   *   @copy copied data which will be passed to the security module.
>>>   *   Returns 0 if the copy was successful.
>>> + * @sb_do_mnt_opts_match:
>>> + *   Determine if the existing mount options are compatible with the=
 new
>>> + *   mount options being used.
>>> + *   @sb superblock being compared
>>> + *   @mnt_opts new mount options
>>> + *   Return 1 if options are the same.
>> This is inconsistent with the convention for returns from LSM interfac=
es.
>> Return 0 on success and -ESOMETHINGOROTHER if the operation would be
>> denied. Your implementation of security_sb_do_mnt_opts_match() will
>> always return 0 if there's no module supplying the hook. I seriously
>> doubt that you want the mounts to fail 100% of the time if there isn't=

>> an LSM.
> The mounts are not failing. This allows a file system to decide
> whether or not to share the superblocks or not.=20

OK. I hadn't gotten that far.

> Ok a good point, I
> haven't tested building a kernel where there is LSM but SElinux is not
> compiled into the kernel (ie, the only user of that hook).

That's a pretty important configuration.

>  I'm not
> sure that's a possible or an interesting option.

Yeah, it's both possible and interesting. Every Ubuntu, Tizen
and most Yocto Project based systems will be affected. There
are many use cases for which SELinux is not the best choice,
at least in this reviewer's experience.

> Ok, I can flip the returns, I wasn't aware that SElinux is so
> restrictive in its return function meaning.

You need to be aware that the LSM infrastructure, while
heavily influenced by SELinux, is not defined by SELinux.

>  Returning 0 on a success
> when you are looking for a match/capability seems backwards.

It is an artifact of the system internal convention of
returning 0 on success and an error code if something goes
wrong. There is one way to succeed, but many ways to fail.

>> Also, "options are the same" isn't the right description, even for
>> SELinux.
> Again compatible is better, I can change it.
>
>>>   * @sb_remount:
>>>   *   Extracts security system specific mount options and verifies no=
 changes
>>>   *   are being made to those options.
>>> diff --git a/include/linux/security.h b/include/linux/security.h
>>> index c35ea0ffccd9..07026db7304d 100644
>>> --- a/include/linux/security.h
>>> +++ b/include/linux/security.h
>>> @@ -291,6 +291,7 @@ int security_sb_alloc(struct super_block *sb);
>>>  void security_sb_free(struct super_block *sb);
>>>  void security_free_mnt_opts(void **mnt_opts);
>>>  int security_sb_eat_lsm_opts(char *options, void **mnt_opts);
>>> +int security_sb_do_mnt_opts_match(struct super_block *sb, void *mnt_=
opts);
>>>  int security_sb_remount(struct super_block *sb, void *mnt_opts);
>>>  int security_sb_kern_mount(struct super_block *sb);
>>>  int security_sb_show_options(struct seq_file *m, struct super_block =
*sb);
>>> diff --git a/security/security.c b/security/security.c
>>> index 7b09cfbae94f..dae380916c6a 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -890,6 +890,13 @@ int security_sb_eat_lsm_opts(char *options, void=
 **mnt_opts)
>>>  }
>>>  EXPORT_SYMBOL(security_sb_eat_lsm_opts);
>>>
>>> +int security_sb_do_mnt_opts_match(struct super_block *sb,
>>> +                              void *mnt_opts)
>>> +{
>>> +     return call_int_hook(sb_do_mnt_opts_match, 0, sb, mnt_opts);
>>> +}
>>> +EXPORT_SYMBOL(security_sb_do_mnt_opts_match);
>>> +
>>>  int security_sb_remount(struct super_block *sb,
>>>                       void *mnt_opts)
>>>  {
>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 644b17ec9e63..aaa3a725da94 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -2656,6 +2656,59 @@ static int selinux_sb_eat_lsm_opts(char *optio=
ns, void **mnt_opts)
>>>       return rc;
>>>  }
>>>
>>> +static int selinux_sb_do_mnt_opts_match(struct super_block *sb, void=
 *mnt_opts)
>>> +{
>>> +     struct selinux_mnt_opts *opts =3D mnt_opts;
>>> +     struct superblock_security_struct *sbsec =3D sb->s_security;
>>> +     u32 sid;
>>> +     int rc;
>>> +
>>> +     /* superblock not initialized (i.e. no options) - reject if any=

>>> +      * options specified, otherwise accept
>>> +      */
>>> +     if (!(sbsec->flags & SE_SBINITIALIZED))
>>> +             return opts ? 0 : 1;
>>> +
>>> +     /* superblock initialized and no options specified - reject if
>>> +      * superblock has any options set, otherwise accept
>>> +      */
>>> +     if (!opts)
>>> +             return (sbsec->flags & SE_MNTMASK) ? 0 : 1;
>>> +
>>> +     if (opts->fscontext) {
>>> +             rc =3D parse_sid(sb, opts->fscontext, &sid);
>>> +             if (rc)
>>> +                     return 0;
>>> +             if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid))
>>> +                     return 0;
>>> +     }
>>> +     if (opts->context) {
>>> +             rc =3D parse_sid(sb, opts->context, &sid);
>>> +             if (rc)
>>> +                     return 0;
>>> +             if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid,=
 sid))
>>> +                     return 0;
>>> +     }
>>> +     if (opts->rootcontext) {
>>> +             struct inode_security_struct *root_isec;
>>> +
>>> +             root_isec =3D backing_inode_security(sb->s_root);
>>> +             rc =3D parse_sid(sb, opts->rootcontext, &sid);
>>> +             if (rc)
>>> +                     return 0;
>>> +             if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, =
sid))
>>> +                     return 0;
>>> +     }
>>> +     if (opts->defcontext) {
>>> +             rc =3D parse_sid(sb, opts->defcontext, &sid);
>>> +             if (rc)
>>> +                     return 0;
>>> +             if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, s=
id))
>>> +                     return 0;
>>> +     }
>>> +     return 1;
>>> +}
>>> +
>>>  static int selinux_sb_remount(struct super_block *sb, void *mnt_opts=
)
>>>  {
>>>       struct selinux_mnt_opts *opts =3D mnt_opts;
>>> @@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[=
] __lsm_ro_after_init =3D {
>>>
>>>       LSM_HOOK_INIT(sb_free_security, selinux_sb_free_security),
>>>       LSM_HOOK_INIT(sb_free_mnt_opts, selinux_free_mnt_opts),
>>> +     LSM_HOOK_INIT(sb_do_mnt_opts_match, selinux_sb_do_mnt_opts_matc=
h),
>>>       LSM_HOOK_INIT(sb_remount, selinux_sb_remount),
>>>       LSM_HOOK_INIT(sb_kern_mount, selinux_sb_kern_mount),
>>>       LSM_HOOK_INIT(sb_show_options, selinux_sb_show_options),