Message-ID: <20240813.la2Aiyico3lo@digikod.net>
Date: Tue, 13 Aug 2024 12:05:06 +0200
From: Mickaël Salaün <mic@...ikod.net>
To: Paul Moore <paul@...l-moore.com>
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>
Subject: Re: [PATCH v2] fs,security: Fix file_set_fowner LSM hook
 inconsistencies

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
why the v1 entirely removed the LSM hook, which is now useless.

Also, f_modown() is renamed to __f_setown().

> 
> 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,
it saves memory, it fixes the LSM/VFS inconsistency, it guarantees
that the VFS semantic is always visible to each LSMs thanks to the use
of the same f_owner.cred, and it avoids LSM mistakes (except if an LSM
implements the now-useless hook).

Powered by blists - more mailing lists