Message-ID: <CAG48ez1fVS=Hg0szXxQym9Yfw4Pgs1THeviXO7wLXbC2-YrLEg@mail.gmail.com>
Date: Mon, 12 Aug 2024 15:09:08 +0200
From: Jann Horn <jannh@...gle.com>
To: Paul Moore <paul@...l-moore.com>
Cc: Mickaël Salaün <mic@...ikod.net>, 
	Christian Brauner <brauner@...nel.org>, Al Viro <viro@...iv.linux.org.uk>, 
	Casey Schaufler <casey@...aufler-ca.com>, Tahera Fahimi <fahimitahera@...il.com>, gnoack@...gle.com, 
	jmorris@...ei.org, serge@...lyn.com, linux-security-module@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add
 signal control)

On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@...l-moore.com> wrote:
>
> On Fri, Aug 9, 2024 at 10:01 AM Jann Horn <jannh@...gle.com> wrote:
> > On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@...ikod.net> wrote:
> > > Talking about f_modown() and security_file_set_fowner(), it looks like
> > > there are some issues:
> > >
> > > On Fri, Aug 09, 2024 at 02:44:06PM +0200, Jann Horn wrote:
> > > > On Fri, Aug 9, 2024 at 12:59 PM Mickaël Salaün <mic@...ikod.net> wrote:
> > >
> > > [...]
> > >
> > > > > BTW, I don't understand why neither SELinux nor Smack use (explicit)
> > > > > atomic operations nor lock.
> > > >
> > > > Yeah, I think they're sloppy and kinda wrong - but it sorta works in
> > > > practice mostly because they don't have to do any refcounting around
> > > > this?
> > > >
> > > > > And it looks weird that
> > > > > security_file_set_fowner() isn't called by f_modown() with the same
> > > > > locking to avoid races.
> > > >
> > > > True. I imagine maybe the thought behind this design could have been
> > > > that LSMs should have their own locking, and that calling an LSM hook
> > > > with IRQs off is a little weird? But the way the LSMs actually use the
> > > > hook now, it might make sense to call the LSM with the lock held and
> > > > IRQs off...
> > > >
> > >
> > > Would it be OK (for VFS, SELinux, and Smack maintainers) to move the
> > > security_file_set_fowner() call into f_modown(), especially where
> > > UID/EUID are populated.  That would only call security_file_set_fowner()
> > > when the fown is actually set, which I think could also fix a bug for
> > > SELinux and Smack.
> > >
> > > Could we replace the uid and euid fields with a pointer to the current
> > > credentials?  This would enables LSMs to not copy the same kind of
> > > credential informations and save some memory, simplify credential
> > > management, and improve consistency.
> >
> > To clarify: These two paragraphs are supposed to be two alternative
> > options, right? One option is to call security_file_set_fowner() with
> > the lock held, the other option is to completely rip out the
> > security_file_set_fowner() hook and instead let the VFS provide LSMs
> > with the creds they need for the file_send_sigiotask hook?
>
> I'm not entirely clear on what is being proposed either.  Some quick
> pseudo code might do wonders here to help clarify things.
>
> From a LSM perspective I suspect we are always going to need some sort
> of hook in the F_SETOWN code path as the LSM needs to potentially
> capture state/attributes/something-LSM-specific at that
> context/point-in-time.

The only thing LSMs currently do there is capture state from
current->cred. So if the VFS takes care of capturing current->cred
there, we should be able to rip out all the file_set_fowner stuff.
Something like this (totally untested):

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 300e5d9ad913..17f159bf625f 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -98,8 +98,9 @@ static void f_modown(struct file *filp, struct pid
*pid, enum pid_type type,

                 if (pid) {
                         const struct cred *cred = current_cred();
-                        filp->f_owner.uid = cred->uid;
-                        filp->f_owner.euid = cred->euid;
+                        if (filp->f_owner.owner_cred)
+                                put_cred(filp->f_owner.owner_cred);
+                        filp->f_owner.owner_cred = get_current_cred();
                 }
         }
         write_unlock_irq(&filp->f_owner.lock);
@@ -108,7 +109,6 @@ static void f_modown(struct file *filp, struct pid
*pid, enum pid_type type,
 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);
diff --git a/fs/file_table.c b/fs/file_table.c
index ca7843dde56d..440796fc8e91 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -426,6 +426,8 @@ static void __fput(struct file *file)
         }
         fops_put(file->f_op);
         put_pid(file->f_owner.pid);
+        if (file->f_owner.owner_cred)
+                put_cred(file->f_owner.owner_cred);
         put_file_access(file);
         dput(dentry);
         if (unlikely(mode & FMODE_NEED_UNMOUNT))
diff --git a/include/linux/fs.h b/include/linux/fs.h
index fd34b5755c0b..43bfad373bf9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -950,7 +950,7 @@ struct fown_struct {
         rwlock_t lock;          /* protects pid, uid, euid fields */
         struct pid *pid;        /* pid or -pgrp where SIGIO should be sent */
         enum pid_type pid_type;        /* Kind of process group SIGIO
should be sent to */
-        kuid_t uid, euid;        /* uid/euid of process setting the owner */
+        const struct cred __rcu *owner_cred;
         int signum;                /* posix.1b rt signal to be
delivered on IO */
 };

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 855db460e08b..2c0935dd079e 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -197,7 +197,6 @@ LSM_HOOK(int, 0, file_mprotect, struct vm_area_struct *vma,
 LSM_HOOK(int, 0, file_lock, struct file *file, unsigned int cmd)
 LSM_HOOK(int, 0, file_fcntl, struct file *file, unsigned int cmd,
          unsigned long arg)
-LSM_HOOK(void, LSM_RET_VOID, file_set_fowner, struct file *file)
 LSM_HOOK(int, 0, file_send_sigiotask, struct task_struct *tsk,
          struct fown_struct *fown, int sig)
 LSM_HOOK(int, 0, file_receive, struct file *file)
diff --git a/include/linux/security.h b/include/linux/security.h
index 1390f1efb4f0..3343db05fa2e 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -1079,11 +1079,6 @@ static inline int security_file_fcntl(struct
file *file, unsigned int cmd,
         return 0;
 }

-static inline void security_file_set_fowner(struct file *file)
-{
-        return;
-}
-
 static inline int security_file_send_sigiotask(struct task_struct *tsk,
                                                struct fown_struct *fown,
                                                int sig)
diff --git a/security/security.c b/security/security.c
index 8cee5b6c6e6d..a53d8d7fe815 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2924,20 +2924,6 @@ int security_file_fcntl(struct file *file,
unsigned int cmd, unsigned long arg)
         return call_int_hook(file_fcntl, file, cmd, arg);
 }

-/**
- * security_file_set_fowner() - Set the file owner info in the LSM blob
- * @file: the file
- *
- * Save owner security information (typically from current->security) in
- * file->f_security for later use by the send_sigiotask hook.
- *
- * Return: Returns 0 on success.
- */
-void security_file_set_fowner(struct file *file)
-{
-        call_void_hook(file_set_fowner, file);
-}
-
 /**
  * security_file_send_sigiotask() - Check if sending SIGIO/SIGURG is allowed
  * @tsk: target task
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 55c78c318ccd..37675d280837 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3649,7 +3649,6 @@ static int selinux_file_alloc_security(struct file *file)
         u32 sid = current_sid();

         fsec->sid = sid;
-        fsec->fown_sid = sid;

         return 0;
 }
@@ -3923,24 +3922,16 @@ static int selinux_file_fcntl(struct file
*file, unsigned int cmd,
         return err;
 }

-static void selinux_file_set_fowner(struct file *file)
-{
-        struct file_security_struct *fsec;
-
-        fsec = selinux_file(file);
-        fsec->fown_sid = current_sid();
-}
-
 static int selinux_file_send_sigiotask(struct task_struct *tsk,
                                        struct fown_struct *fown, int signum)
 {
-        struct file *file;
+        /* struct fown_struct is never outside the context of a struct file */
+        struct file *file = container_of(fown, struct file, f_owner);
         u32 sid = task_sid_obj(tsk);
         u32 perm;
         struct file_security_struct *fsec;
-
-        /* struct fown_struct is never outside the context of a struct file */
-        file = container_of(fown, struct file, f_owner);
+        struct cred_struct *fown_cred = rcu_dereference(fown->owner_cred);
+        u32 fown_sid = cred_sid(fown_cred ?: file->f_cred);

         fsec = selinux_file(file);

@@ -3949,7 +3940,7 @@ static int selinux_file_send_sigiotask(struct
task_struct *tsk,
         else
                 perm = signal_to_av(signum);

-        return avc_has_perm(fsec->fown_sid, sid,
+        return avc_has_perm(fown_sid, sid,
                             SECCLASS_PROCESS, perm, NULL);
 }

diff --git a/security/selinux/include/objsec.h
b/security/selinux/include/objsec.h
index dea1d6f3ed2d..d55b7f8d3a3d 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -56,7 +56,6 @@ struct inode_security_struct {

 struct file_security_struct {
         u32 sid; /* SID of open file description */
-        u32 fown_sid; /* SID of file owner (for SIGIO) */
         u32 isid; /* SID of inode at the time of file open */
         u32 pseqno; /* Policy seqno at the time of file open */
 };
diff --git a/security/smack/smack.h b/security/smack/smack.h
index 041688e5a77a..06bac00cc796 100644
--- a/security/smack/smack.h
+++ b/security/smack/smack.h
@@ -328,12 +328,6 @@ static inline struct task_smack *smack_cred(const
struct cred *cred)
         return cred->security + smack_blob_sizes.lbs_cred;
 }

-static inline struct smack_known **smack_file(const struct file *file)
-{
-        return (struct smack_known **)(file->f_security +
-                                       smack_blob_sizes.lbs_file);
-}
-
 static inline struct inode_smack *smack_inode(const struct inode *inode)
 {
         return inode->i_security + smack_blob_sizes.lbs_inode;
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 4164699cd4f6..02caa8b9d456 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1675,26 +1675,6 @@ static void smack_inode_getsecid(struct inode
*inode, u32 *secid)
  * label changing that SELinux does.
  */

-/**
- * smack_file_alloc_security - assign a file security blob
- * @file: the object
- *
- * The security blob for a file is a pointer to the master
- * label list, so no allocation is done.
- *
- * f_security is the owner security information. It
- * isn't used on file access checks, it's for send_sigio.
- *
- * Returns 0
- */
-static int smack_file_alloc_security(struct file *file)
-{
-        struct smack_known **blob = smack_file(file);
-
-        *blob = smk_of_current();
-        return 0;
-}
-
 /**
  * smack_file_ioctl - Smack check on ioctls
  * @file: the object
@@ -1913,18 +1893,6 @@ static int smack_mmap_file(struct file *file,
         return rc;
 }

-/**
- * smack_file_set_fowner - set the file security blob value
- * @file: object in question
- *
- */
-static void smack_file_set_fowner(struct file *file)
-{
-        struct smack_known **blob = smack_file(file);
-
-        *blob = smk_of_current();
-}
-
 /**
  * smack_file_send_sigiotask - Smack on sigio
  * @tsk: The target task
@@ -1946,6 +1914,7 @@ static int smack_file_send_sigiotask(struct
task_struct *tsk,
         struct file *file;
         int rc;
         struct smk_audit_info ad;
+        struct cred_struct *fown_cred = rcu_dereference(fown->owner_cred);

         /*
          * struct fown_struct is never outside the context of a struct file
@@ -1953,8 +1922,7 @@ static int smack_file_send_sigiotask(struct
task_struct *tsk,
         file = container_of(fown, struct file, f_owner);

         /* we don't log here as rc can be overriden */
-        blob = smack_file(file);
-        skp = *blob;
+        skp = smk_of_task(fown_cred ?: file->f_cred);
         rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
         rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc);

@@ -5045,7 +5013,6 @@ static int smack_uring_cmd(struct io_uring_cmd *ioucmd)

 struct lsm_blob_sizes smack_blob_sizes __ro_after_init = {
         .lbs_cred = sizeof(struct task_smack),
-        .lbs_file = sizeof(struct smack_known *),
         .lbs_inode = sizeof(struct inode_smack),
         .lbs_ipc = sizeof(struct smack_known *),
         .lbs_msg_msg = sizeof(struct smack_known *),
@@ -5104,7 +5071,6 @@ static struct security_hook_list smack_hooks[]
__ro_after_init = {
         LSM_HOOK_INIT(file_fcntl, smack_file_fcntl),
         LSM_HOOK_INIT(mmap_file, smack_mmap_file),
         LSM_HOOK_INIT(mmap_addr, cap_mmap_addr),
-        LSM_HOOK_INIT(file_set_fowner, smack_file_set_fowner),
         LSM_HOOK_INIT(file_send_sigiotask, smack_file_send_sigiotask),
         LSM_HOOK_INIT(file_receive, smack_file_receive),


> While I think it is okay if we want to
> consider relocating the security_file_set_fowner() within the F_SETOWN
> call path, I don't think we can remove it, even if we add additional
> LSM security blobs.

Powered by blists - more mailing lists