[PATCH 1/2] [security] Add new hook to compare new mount to an existing mount
Casey Schaufler
casey at schaufler-ca.com
Sat Feb 13 00:32:46 UTC 2021
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 at schaufler-ca.com> wrote:
>> On 2/12/2021 1:19 PM, Olga Kornievskaia wrote:
>>> From: Olga Kornievskaia <kolga at 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 at 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_defs.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_block *sb)
>>> LSM_HOOK(void, LSM_RET_VOID, sb_free_security, struct super_block *sb)
>>> 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_block *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 interfaces.
>> 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.
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 *options, 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 = mnt_opts;
>>> + struct superblock_security_struct *sbsec = 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 = 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 = 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 = backing_inode_security(sb->s_root);
>>> + rc = 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 = parse_sid(sb, opts->defcontext, &sid);
>>> + if (rc)
>>> + return 0;
>>> + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid))
>>> + return 0;
>>> + }
>>> + return 1;
>>> +}
>>> +
>>> static int selinux_sb_remount(struct super_block *sb, void *mnt_opts)
>>> {
>>> struct selinux_mnt_opts *opts = mnt_opts;
>>> @@ -6984,6 +7037,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>>>
>>> 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_match),
>>> 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),
More information about the Linux-security-module-archive
mailing list