| Message ID | 20210219222233.20748-1-olga.kornievskaia@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3,security] Add new hook to compare new mount to an existing mount | expand |
On Fri, Feb 19, 2021 at 5:25 PM Olga Kornievskaia <olga.kornievskaia@gmail.com> 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. > > A filesystem can use this new hook to determine if it can share > the an existing superblock with a new superblock for the new mount. > > 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 | 8 +++++ > security/security.c | 7 +++++ > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++++++++ > 5 files changed, 78 insertions(+) ... > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index a19adef1f088..d76aaecfdf0f 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_mnt_opts_compat: > + * Determine if the existing mount options are compatible with the new > + * mount options being used. Full disclosure: I'm a big fan of good documentation, regardless of if it lives in comments or a separate dedicated resource. Looking at the comment above, and the SELinux implementation of this hook below, it appears that the comment is a bit vague; specifically the use of "compatible". Based on the SELinux implementation, "compatible" would seem to equal, do you envision that to be the case for every LSM/security-model? If the answer is yes, then let's say that (and possibly rename the hook to "sb_mnt_opts_equal"). If the answer is no, then I think we need to do a better job explaining what compatibility really means; put yourself in the shoes of someone writing a LSM, what would they need to know to write an implementation for this hook? > + * @sb superblock being compared > + * @mnt_opts new mount options > + * Return 0 if options are compatible.
On Thu, Feb 25, 2021 at 12:53 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Feb 19, 2021 at 5:25 PM Olga Kornievskaia > <olga.kornievskaia@gmail.com> 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. > > > > A filesystem can use this new hook to determine if it can share > > the an existing superblock with a new superblock for the new mount. > > > > 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 | 8 +++++ > > security/security.c | 7 +++++ > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++++++++ > > 5 files changed, 78 insertions(+) > > ... > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > index a19adef1f088..d76aaecfdf0f 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_mnt_opts_compat: > > + * Determine if the existing mount options are compatible with the new > > + * mount options being used. > > Full disclosure: I'm a big fan of good documentation, regardless of if > it lives in comments or a separate dedicated resource. Looking at the > comment above, and the SELinux implementation of this hook below, it > appears that the comment is a bit vague; specifically the use of > "compatible". Based on the SELinux implementation, "compatible" would > seem to equal, do you envision that to be the case for every > LSM/security-model? If the answer is yes, then let's say that (and > possibly rename the hook to "sb_mnt_opts_equal"). If the answer is > no, then I think we need to do a better job explaining what > compatibility really means; put yourself in the shoes of someone > writing a LSM, what would they need to know to write an implementation > for this hook? That's is tough to do as it is vague. All I was doing was fixing a bug. Selinux didn't allow a new mount because it had a different security context. What that translates to for the new hook, is up to the LSM module whether it would need the options to be exactly the same or if they can be slightly different but yet compatible this is really up to the LSM. Do you care to suggest wording to use? It is hard to find words that somebody else is looking for but one is unable to provide them. > > > + * @sb superblock being compared > > + * @mnt_opts new mount options > > + * Return 0 if options are compatible. > > -- > paul moore > www.paul-moore.com
On 2/25/2021 10:03 AM, Olga Kornievskaia wrote: > On Thu, Feb 25, 2021 at 12:53 PM Paul Moore <paul@paul-moore.com> wrote: >> On Fri, Feb 19, 2021 at 5:25 PM Olga Kornievskaia >> <olga.kornievskaia@gmail.com> 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. >>> >>> A filesystem can use this new hook to determine if it can share >>> the an existing superblock with a new superblock for the new mount. >>> >>> 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 | 8 +++++ >>> security/security.c | 7 +++++ >>> security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++++++++ >>> 5 files changed, 78 insertions(+) >> ... >> >>> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h >>> index a19adef1f088..d76aaecfdf0f 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_mnt_opts_compat: >>> + * Determine if the existing mount options are compatible with the new >>> + * mount options being used. >> Full disclosure: I'm a big fan of good documentation, regardless of if >> it lives in comments or a separate dedicated resource. Looking at the >> comment above, and the SELinux implementation of this hook below, it >> appears that the comment is a bit vague; specifically the use of >> "compatible". Based on the SELinux implementation, "compatible" would >> seem to equal, do you envision that to be the case for every >> LSM/security-model? The original implementation did use sb_mnt_opts_equal(). The change to "compatible" was my suggestion. Smack has multiple mount options, and while I haven't actually delved into how you would have compatible but different mount options, I think it's possible. That's why I think that "equal" isn't a good name for the function. >> If the answer is yes, then let's say that (and >> possibly rename the hook to "sb_mnt_opts_equal"). If the answer is >> no, then I think we need to do a better job explaining what >> compatibility really means; put yourself in the shoes of someone >> writing a LSM, what would they need to know to write an implementation >> for this hook? > That's is tough to do as it is vague. All I was doing was fixing a > bug. Selinux didn't allow a new mount because it had a different > security context. What that translates to for the new hook, is up to > the LSM module whether it would need the options to be exactly the > same or if they can be slightly different but yet compatible this is > really up to the LSM. > > Do you care to suggest wording to use? It is hard to find words that > somebody else is looking for but one is unable to provide them. > >>> + * @sb superblock being compared >>> + * @mnt_opts new mount options >>> + * Return 0 if options are compatible. >> -- >> paul moore >> www.paul-moore.com
On Thu, Feb 25, 2021 at 1:03 PM Olga Kornievskaia <olga.kornievskaia@gmail.com> wrote: > On Thu, Feb 25, 2021 at 12:53 PM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Feb 19, 2021 at 5:25 PM Olga Kornievskaia > > <olga.kornievskaia@gmail.com> 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. > > > > > > A filesystem can use this new hook to determine if it can share > > > the an existing superblock with a new superblock for the new mount. > > > > > > 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 | 8 +++++ > > > security/security.c | 7 +++++ > > > security/selinux/hooks.c | 56 +++++++++++++++++++++++++++++++++++ > > > 5 files changed, 78 insertions(+) > > > > ... > > > > > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > > > index a19adef1f088..d76aaecfdf0f 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_mnt_opts_compat: > > > + * Determine if the existing mount options are compatible with the new > > > + * mount options being used. > > > > Full disclosure: I'm a big fan of good documentation, regardless of if > > it lives in comments or a separate dedicated resource. Looking at the > > comment above, and the SELinux implementation of this hook below, it > > appears that the comment is a bit vague; specifically the use of > > "compatible". Based on the SELinux implementation, "compatible" would > > seem to equal, do you envision that to be the case for every > > LSM/security-model? If the answer is yes, then let's say that (and > > possibly rename the hook to "sb_mnt_opts_equal"). If the answer is > > no, then I think we need to do a better job explaining what > > compatibility really means; put yourself in the shoes of someone > > writing a LSM, what would they need to know to write an implementation > > for this hook? > > That's is tough to do as it is vague. All I was doing was fixing a > bug. Selinux didn't allow a new mount because it had a different > security context. What that translates to for the new hook, is up to > the LSM module whether it would need the options to be exactly the > same or if they can be slightly different but yet compatible this is > really up to the LSM. > > Do you care to suggest wording to use? It is hard to find words that > somebody else is looking for but one is unable to provide them. I didn't have anything particular in mind, I just *really* don't like the ambiguity around "compatible". Perhaps we can take away some of the ambiguity by providing some more explanation, how about something like this: "Determine if the new mount options in @mnt_opts are allowed given the existing mounted filesystem at @sb." ... it's a pretty minor change, I'll readily admit that, but it exchanges "compatible" for "allowed" which I *think* makes it a bit more concrete.
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 7aaa753b8608..1b12a5266a51 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_mnt_opts_compat, struct super_block *sb, void *mnt_opts) 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..d76aaecfdf0f 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_mnt_opts_compat: + * 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 0 if options are compatible. * @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..50db3d5d1608 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_mnt_opts_compat(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); @@ -635,6 +636,13 @@ static inline int security_sb_remount(struct super_block *sb, return 0; } +static inline int security_sb_mnt_opts_compat(struct super_block *sb, + void *mnt_opts) +{ + return 0; +} + + static inline int security_sb_kern_mount(struct super_block *sb) { return 0; diff --git a/security/security.c b/security/security.c index 7b09cfbae94f..56cf5563efde 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_mnt_opts_compat(struct super_block *sb, + void *mnt_opts) +{ + return call_int_hook(sb_mnt_opts_compat, 0, sb, mnt_opts); +} +EXPORT_SYMBOL(security_sb_mnt_opts_compat); + 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..afee3a222a0e 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2656,6 +2656,61 @@ static int selinux_sb_eat_lsm_opts(char *options, void **mnt_opts) return rc; } +static int selinux_sb_mnt_opts_compat(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 ? 1 : 0; + + /* + * Superblock initialized and no options specified - reject if + * superblock has any options set, otherwise accept. + */ + if (!opts) + return (sbsec->flags & SE_MNTMASK) ? 1 : 0; + + if (opts->fscontext) { + rc = parse_sid(sb, opts->fscontext, &sid); + if (rc) + return 1; + if (bad_option(sbsec, FSCONTEXT_MNT, sbsec->sid, sid)) + return 1; + } + if (opts->context) { + rc = parse_sid(sb, opts->context, &sid); + if (rc) + return 1; + if (bad_option(sbsec, CONTEXT_MNT, sbsec->mntpoint_sid, sid)) + return 1; + } + 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 1; + if (bad_option(sbsec, ROOTCONTEXT_MNT, root_isec->sid, sid)) + return 1; + } + if (opts->defcontext) { + rc = parse_sid(sb, opts->defcontext, &sid); + if (rc) + return 1; + if (bad_option(sbsec, DEFCONTEXT_MNT, sbsec->def_sid, sid)) + return 1; + } + return 0; +} + static int selinux_sb_remount(struct super_block *sb, void *mnt_opts) { struct selinux_mnt_opts *opts = mnt_opts; @@ -6984,6 +7039,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_mnt_opts_compat, selinux_sb_mnt_opts_compat), 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),