[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240908.jeim4Aif3Fee@digikod.net>
Date: Sun, 8 Sep 2024 20:11:07 +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>,
Mateusz Guzik <mjguzik@...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 v3 1/2] fs: Fix file_set_fowner LSM hook inconsistencies
On Wed, Aug 21, 2024 at 12:32:17PM -0400, Paul Moore wrote:
> On Wed, Aug 21, 2024 at 5:56 AM 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 always called, ignoring the VFS logic which
> > may not actually change the process that handles SIGIO (e.g. TUN, TTY,
> > dnotify), nor update the related UID/EUID.
> >
> > Moreover, because security_file_set_fowner() was called 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.
> >
> > This change makes sure the LSM states are always in sync with the VFS
> > state by moving the security_file_set_fowner() call close to the
> > UID/EUID updates and using the same f_owner.lock .
> >
> > 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>
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Mickaël Salaün <mic@...ikod.net>
> > ---
> >
> > Changes since v2:
> > https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net
> > - Only keep the LSM hook move.
> >
> > 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 | 14 ++++----------
> > 1 file changed, 4 insertions(+), 10 deletions(-)
>
> This looks reasonable to me, and fixes a potential problem with
> existing LSMs. Unless I hear any strong objections I'll plan to merge
> this, and patch 2/2, into the LSM tree tomorrow.
I didn't see these patches in -next, did I miss something?
Landlock will use this hook really soon and it would make it much easier
if these patches where upstream before.
>
> > diff --git a/fs/fcntl.c b/fs/fcntl.c
> > index 300e5d9ad913..c28dc6c005f1 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) {
> > @@ -98,19 +98,13 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
> >
> > if (pid) {
> > const struct cred *cred = current_cred();
> > + security_file_set_fowner(filp);
> > filp->f_owner.uid = cred->uid;
> > filp->f_owner.euid = cred->euid;
> > }
> > }
> > write_unlock_irq(&filp->f_owner.lock);
> > }
> > -
> > -void __f_setown(struct file *filp, struct pid *pid, enum pid_type type,
> > - int force)
> > -{
> > - security_file_set_fowner(filp);
> > - f_modown(filp, pid, type, force);
> > -}
> > EXPORT_SYMBOL(__f_setown);
> >
> > int f_setown(struct file *filp, int who, int force)
> > @@ -146,7 +140,7 @@ EXPORT_SYMBOL(f_setown);
> >
> > void f_delown(struct file *filp)
> > {
> > - f_modown(filp, NULL, PIDTYPE_TGID, 1);
> > + __f_setown(filp, NULL, PIDTYPE_TGID, 1);
> > }
> >
> > pid_t f_getown(struct file *filp)
> > --
> > 2.46.0
>
> --
> paul-moore.com
>
Powered by blists - more mailing lists