linux-kernel - Re: [PATCH v3 1/2] fs: Fix file_set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHC9VhQ7e50Ya4BNoF-xM2y+MDMW3i_SRPVcZkDZ2vdEMNtk7Q@mail.gmail.com>
Date: Wed, 21 Aug 2024 12:32:17 -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>, 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 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.

> 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