Re: [PATCH v3 1/3] [security] Add new hook to compare new mount to an existing mount

[Paul Moore <paul@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.

-- 
paul moore
www.paul-moore.com