Message-ID: <CAHC9VhR-jbQQpb6OZjtDyhmkq3gb5GLkt87tfUBQM84uG-q1bQ@mail.gmail.com>
Date: Tue, 13 Aug 2024 19:39:45 -0400
From: Paul Moore <paul@...l-moore.com>
To: Mickaël Salaün <mic@...ikod.net>
Cc: Christian Brauner <brauner@...nel.org>, linux-fsdevel@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org, 
	selinux@...r.kernel.org, Jan Kara <jack@...e.cz>, 
	Tahera Fahimi <fahimitahera@...il.com>, Al Viro <viro@...iv.linux.org.uk>, 
	Casey Schaufler <casey@...aufler-ca.com>, James Morris <jmorris@...ei.org>, Jann Horn <jannh@...gle.com>, 
	Ondrej Mosnacek <omosnace@...hat.com>, "Serge E . Hallyn" <serge@...lyn.com>, 
	Stephen Smalley <stephen.smalley.work@...il.com>, Mateusz Guzik <mjguzik@...il.com>
Subject: Re: [PATCH v2] fs,security: Fix file_set_fowner LSM hook inconsistencies

On Tue, Aug 13, 2024 at 2:28 PM Mickaël Salaün <mic@...ikod.net> wrote:
> On Tue, Aug 13, 2024 at 11:04:13AM -0400, Paul Moore wrote:
> > On Tue, Aug 13, 2024 at 6:05 AM Mickaël Salaün <mic@...ikod.net> wrote:
> > > On Mon, Aug 12, 2024 at 06:26:58PM -0400, Paul Moore wrote:
> > > > On Mon, Aug 12, 2024 at 1:44 PM Mickaël Salaün <mic@...ikod.net> wrote:
> > > > >
> > > > > The fcntl's F_SETOWN command sets the process that handle SIGIO/SIGURG
> > > > > for the related file descriptor.  Before this change, the
> > > > > file_set_fowner LSM hook was used to store this information.  However,
> > > > > there are three issues with this approach:
> > > > >
> > > > > - Because security_file_set_fowner() only get one argument, all hook
> > > > >   implementations ignore the VFS logic which may not actually change the
> > > > >   process that handles SIGIO (e.g. TUN, TTY, dnotify).
> > > > >
> > > > > - Because security_file_set_fowner() is called before f_modown() without
> > > > >   lock (e.g. f_owner.lock), concurrent F_SETOWN commands could result to
> > > > >   a race condition and inconsistent LSM states (e.g. SELinux's fown_sid)
> > > > >   compared to struct fown_struct's UID/EUID.
> > > > >
> > > > > - Because the current hook implementations does not use explicit atomic
> > > > >   operations, they may create inconsistencies.  It would help to
> > > > >   completely remove this constraint, as well as the requirements of the
> > > > >   RCU read-side critical section for the hook.
> > > > >
> > > > > Fix these issues by replacing f_owner.uid and f_owner.euid with a new
> > > > > f_owner.cred [1].  This also saves memory by removing dedicated LSM
> > > > > blobs, and simplifies code by removing file_set_fowner hook
> > > > > implementations for SELinux and Smack.
> > > > >
> > > > > This changes enables to remove the smack_file_alloc_security
> > > > > implementation, Smack's file blob, and SELinux's
> > > > > file_security_struct->fown_sid field.
> > > > >
> > > > > As for the UID/EUID, f_owner.cred is not always updated.  Move the
> > > > > file_set_fowner hook to align with the VFS semantic.  This hook does not
> > > > > have user anymore [2].
> > > > >
> > > > > Before this change, f_owner's UID/EUID were initialized to zero
> > > > > (i.e. GLOBAL_ROOT_UID), but to simplify code, f_owner's cred is now
> > > > > initialized with the file descriptor creator's credentials (i.e.
> > > > > file->f_cred), which is more consistent and simplifies LSMs logic.  The
> > > > > sigio_perm()'s semantic does not need any change because SIGIO/SIGURG
> > > > > are only sent when a process is explicitly set with __f_setown().
> > > > >
> > > > > Rename f_modown() to __f_setown() to simplify code.
> > > > >
> > > > > Cc: Al Viro <viro@...iv.linux.org.uk>
> > > > > Cc: Casey Schaufler <casey@...aufler-ca.com>
> > > > > Cc: Christian Brauner <brauner@...nel.org>
> > > > > Cc: James Morris <jmorris@...ei.org>
> > > > > Cc: Jann Horn <jannh@...gle.com>
> > > > > Cc: Ondrej Mosnacek <omosnace@...hat.com>
> > > > > Cc: Paul Moore <paul@...l-moore.com>
> > > > > Cc: Serge E. Hallyn <serge@...lyn.com>
> > > > > Cc: Stephen Smalley <stephen.smalley.work@...il.com>
> > > > > Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1]
> > > > > Link: https://lore.kernel.org/r/CAHC9VhQY+H7n2zCn8ST0Vu672UA=_eiUikRDW2sUDSN3c=gVQw@mail.gmail.com [2]
> > > > > Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> > > > > ---
> > > > >
> > > > > Changes since v1:
> > > > > https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net
> > > > > - Add back the file_set_fowner hook (but without user) as
> > > > >   requested by Paul, but move it for consistency.
> > > > > ---
> > > > >  fs/fcntl.c                        | 42 +++++++++++++++----------------
> > > > >  fs/file_table.c                   |  3 +++
> > > > >  include/linux/fs.h                |  2 +-
> > > > >  security/security.c               |  5 +++-
> > > > >  security/selinux/hooks.c          | 22 +++-------------
> > > > >  security/selinux/include/objsec.h |  1 -
> > > > >  security/smack/smack.h            |  6 -----
> > > > >  security/smack/smack_lsm.c        | 39 +---------------------------
> > > > >  8 files changed, 33 insertions(+), 87 deletions(-)
> > > > >
> > > > > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > > > > index 300e5d9ad913..4217b66a4e99 100644
> > > > > --- a/fs/fcntl.c
> > > > > +++ b/fs/fcntl.c
> > > > > @@ -87,8 +87,8 @@ static int setfl(int fd, struct file * filp, unsigned int arg)
> > > > >         return error;
> > > > >  }
> > > > >
> > > > > -static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> > > > > -                     int force)
> > > > > +void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> > > > > +               int force)
> > > > >  {
> > > > >         write_lock_irq(&filp->f_owner.lock);
> > > > >         if (force || !filp->f_owner.pid) {
> > > > > @@ -97,20 +97,15 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> > > > >                 filp->f_owner.pid_type = type;
> > > > >
> > > > >                 if (pid) {
> > > > > -                       const struct cred *cred = current_cred();
> > > > > -                       filp->f_owner.uid = cred->uid;
> > > > > -                       filp->f_owner.euid = cred->euid;
> > > > > +                       security_file_set_fowner(filp);
> > > > > +                       put_cred(rcu_replace_pointer(
> > > > > +                               filp->f_owner.cred,
> > > > > +                               get_cred_rcu(current_cred()),
> > > > > +                               lockdep_is_held(&filp->f_owner.lock)));
> > > > >                 }
> > > > >         }
> > > > >         write_unlock_irq(&filp->f_owner.lock);
> > > > >  }
> > > >
> > > > Looking at this quickly, why can't we accomplish pretty much the same
> > > > thing by moving the security_file_set_fowner() into f_modown (as
> > > > you've done above) and leveraging the existing file->f_security field
> > > > as Smack and SELinux do today?  I'm seeing a lot of churn to get a
> > > > cred pointer into fown_struct which doesn't seem to offer that much
> > > > additional value.
> > >
> > > As explained in the commit message, this patch removes related LSM
> > > (sub)blobs because they are duplicates of what's referenced by the new
> > > f_owner.cred, which is a more generic approach and saves memory.
> >
> > That's not entirely correct.  While yes you do remove the need for a
> > Smack entry in file->f_security, there is still a need for the SELinux
> > entry in file->f_security no matter what you do, and since the LSM
> > framework handles the LSM security blob allocations, on systems where
> > SELinux is enabled you are going to do a file->f_security allocation
> > regardless.
>
> That's why I used "(sub)" blob, for the case of SELinux that "only" drop
> a field.

Your choice of phrasing was misleading in my opinion.

> > While a cred based approach may be more generic from a traditional
> > UID/GID/etc. perspective, file->f_security is always going to be more
> > generic from a LSM perspective as the LSM has more flexibility about
> > what is placed into that blob.  Yes, the LSM can also place data into
> > the cred struct, but that is used across a wide variety of kernel
> > objects and placing file specific data in there could needlessly
> > increase the size of the cred struct.
>
> Yes, it could, but that is not the case with the current implementations
> (SELinux and Smack). I understand that it could be useful though.

Please keep that last sentence in mind.

> > > > From what I can see this seems really focused on adding a cred
> > > > reference when it isn't clear an additional one is needed.  If a new
> > > > cred reference *is* needed, please provide an explanation as to why;
> > > > reading the commit description this isn't clear.  Of course, if I'm
> > > > mistaken, feel free to correct me, although I'm sure all the people on
> > > > the Internet don't need to be told that ;)
> > >
> > > This is a more generic approach that saves memory, sticks to the VFS
> > > semantic, and removes code.  So I'd say it's a performance improvement
> >
> > Considering that additional cred gets/puts are needed I question if
> > there are actually any performance improvements; in some cases I
> > suspect the performance will actually be worse.  On SELinux enabled
> > systems you are still going to do the file->f_security allocation and
> > now you are going to add the cred management operations on top of
> > that.
>
> I was talking about the extra hook calls which are not needed.  The move
> of fown_struct ou of the file struct should limit any credential
> reference performance impact, and Mateusz said he is working on
> improving this part too.

I don't see how where the cred reference live will have any impact,
you still need to get and drop references which will have an impact.
There will always be something.

> > With the move in linux-next to pull fown_struct out of the file
> > struct, I suspect this is not as important as it once may have been.
>
> I was talking about the LSM blobs shrinking, which impacts all opened
> files, independently of moving fown_struct out of the file struct.  I
> think this is not negligible: 32 bits for SELinux + 64 bits for Smack +
> 64 bits for ongoing Landlock support = potentially 128 bits for each
> opened files.

I'm going to skip over the Landlock contribution as that isn't part of
the patchset and to the best of my knowledge that is still a work in
progress and not finalized.

You also forgot to add in the cost of the fown_struct->cred pointer.

I noticed you chose to do your count in bits, likely to make the
numbers look bigger (which is fair), I'm going to do my count in bytes
;) ... So we've got four bytes removed from the SELinux blob, and
eight bytes from the Smack blob, but we add back in another eight
bytes for the new cred pointer, making a net benefit of only four
bytes for each open file.  Considering my concerns around the loss of
flexibility at the LSM layer I don't see this as a worthwhile
tradeoff.

> > > it fixes the LSM/VFS inconsistency
> >
> > Simply moving the security_file_set_fowner() inside the lock protected
> > region should accomplish that too.  Unless you're talking about
> > something else?
>
> Yes, the moving the hook fixes that.
>
> > > it guarantees
> > > that the VFS semantic is always visible to each LSMs thanks to the use
> > > of the same f_owner.cred
> >
> > The existing hooks are designed to make sure that the F_SETOWN
> > operation is visible to the LSM.
>
> This should not change the F_SETOWN case.  Am I missing something?

I don't know, you were talking about making sure the VFS semantics are
visible to the LSM and I was simply suggesting that the existing hooks
do that too.  To be honest, whatever point you are trying to make here
isn't very clear to me.

> > > and it avoids LSM mistakes (except if an LSM implements the now-useless hook).
> >
> > The only mistake I'm seeing is that the call into
> > security_file_set_fowner() is not in the lock protected region, and
> > that is easily corrected.  Forcing the LSM framework to reuse a cred
> > struct has the potential to restrict LSM security models which is
> > something we try very hard not to do.
>
> OK, but is the current approach (i.e. keep the LSM hook and reducing LSM
> blobs size) good for you?  What do you want me to remove from this
> patch?

I agree that the security_file_set_owner() hook needs to be moved.  I
disagree about the value in shifting the LSM framework over to a cred
reference which effectively abandons the existing hook.  My preference
would be to preserve the flexibility of the hook, but move it to the
protected lock location, and continue to leverage the file->f_security
blob as needed for any LSM state.

-- 
paul-moore.com

Powered by blists - more mailing lists