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 [off-list ref] wrote:

quoted

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@zeniv.linux.org.uk>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Christian Brauner <brauner@kernel.org>
Cc: James Morris <jmorris@namei.org>
Cc: Jann Horn <jannh@google.com>
Cc: Ondrej Mosnacek <omosnace@redhat.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Stephen Smalley <stephen.smalley.work@gmail.com>
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---

Changes since v2:
https://lore.kernel.org/r/20240812174421.1636724-1-mic@digikod.net (local)
- Only keep the LSM hook move.

Changes since v1:
https://lore.kernel.org/r/20240812144936.1616628-1-mic@digikod.net (local)
- 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.

quoted

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