f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Mickaël Salaün]

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@digikod.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.

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Jann Horn]

On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@digikod.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@digikod.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?

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Christian Brauner]

On Fri, Aug 09, 2024 at 04:00:41PM GMT, Jann Horn wrote:
> On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@digikod.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@digikod.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 think it would be fine to stash the credentials in struct fown_struct
same as we do for struct file itself. Calling security_file_set_fowner()
with the irq lock held seems suboptimal to me. Plus, this also means one
less LSM hook and that seems like a win too.

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Paul Moore]

On Fri, Aug 9, 2024 at 10:01 AM Jann Horn <jannh@google.com> wrote:
> On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@digikod.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@digikod.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.  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.

-- 
paul-moore.com

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Jann Horn]

On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Fri, Aug 9, 2024 at 10:01 AM Jann Horn <jannh@google.com> wrote:
> > On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@digikod.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@digikod.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.

[PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[Mickaël Salaün]

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 the file_set_fowner LSM hook.

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.  Because the
file_set_fowner hook is removed, the fowner credentials now have the
same semantic as what is used by the VFS.

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@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>
Link: https://lore.kernel.org/r/20240809-explosionsartig-ablesen-b039dbc6ce82@brauner [1]
Signed-off-by: Mickaël Salaün <mic@digikod.net>
---
 fs/fcntl.c                        | 44 +++++++++++++++----------------
 fs/file_table.c                   |  3 +++
 include/linux/fs.h                |  2 +-
 include/linux/lsm_hook_defs.h     |  1 -
 include/linux/security.h          |  1 -
 security/security.c               | 14 ----------
 security/selinux/hooks.c          | 22 +++-------------
 security/selinux/include/objsec.h |  1 -
 security/smack/smack.h            |  6 -----
 security/smack/smack_lsm.c        | 39 +--------------------------
 10 files changed, 29 insertions(+), 104 deletions(-)

diff --git a/fs/fcntl.c b/fs/fcntl.c
index 300e5d9ad913..4832d6b6759c 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) {
@@ -96,21 +96,14 @@ static void f_modown(struct file *filp, struct pid *pid, enum pid_type type,
 		filp->f_owner.pid = get_pid(pid);
 		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;
-		}
+		if (pid)
+			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);
 }
-
-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 +139,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)
@@ -249,13 +242,15 @@ static int f_getowner_uids(struct file *filp, unsigned long arg)
 {
 	struct user_namespace *user_ns = current_user_ns();
 	uid_t __user *dst = (void __user *)arg;
+	const struct cred *fown_cred;
 	uid_t src[2];
 	int err;
 
-	read_lock_irq(&filp->f_owner.lock);
-	src[0] = from_kuid(user_ns, filp->f_owner.uid);
-	src[1] = from_kuid(user_ns, filp->f_owner.euid);
-	read_unlock_irq(&filp->f_owner.lock);
+	rcu_read_lock();
+	fown_cred = rcu_dereference(filp->f_owner->cred);
+	src[0] = from_kuid(user_ns, fown_cred->uid);
+	src[1] = from_kuid(user_ns, fown_cred->euid);
+	rcu_read_unlock();
 
 	err  = put_user(src[0], &dst[0]);
 	err |= put_user(src[1], &dst[1]);
@@ -737,14 +732,17 @@ static const __poll_t band_table[NSIGPOLL] = {
 static inline int sigio_perm(struct task_struct *p,
                              struct fown_struct *fown, int sig)
 {
-	const struct cred *cred;
+	const struct cred *cred, *fown_cred;
 	int ret;
 
 	rcu_read_lock();
+	fown_cred = rcu_dereference(fown->cred);
 	cred = __task_cred(p);
-	ret = ((uid_eq(fown->euid, GLOBAL_ROOT_UID) ||
-		uid_eq(fown->euid, cred->suid) || uid_eq(fown->euid, cred->uid) ||
-		uid_eq(fown->uid,  cred->suid) || uid_eq(fown->uid,  cred->uid)) &&
+	ret = ((uid_eq(fown_cred->euid, GLOBAL_ROOT_UID) ||
+		uid_eq(fown_cred->euid, cred->suid) ||
+		uid_eq(fown_cred->euid, cred->uid) ||
+		uid_eq(fown_cred->uid, cred->suid) ||
+		uid_eq(fown_cred->uid, cred->uid)) &&
 	       !security_file_send_sigiotask(p, fown, sig));
 	rcu_read_unlock();
 	return ret;
diff --git a/fs/file_table.c b/fs/file_table.c
index 4f03beed4737..d28b76aef4f3 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -66,6 +66,7 @@ static inline void file_free(struct file *f)
 	if (likely(!(f->f_mode & FMODE_NOACCOUNT)))
 		percpu_counter_dec(&nr_files);
 	put_cred(f->f_cred);
+	put_cred(f->f_owner.cred);
 	if (unlikely(f->f_mode & FMODE_BACKING)) {
 		path_put(backing_file_user_path(f));
 		kfree(backing_file(f));
@@ -149,9 +150,11 @@ static int init_file(struct file *f, int flags, const struct cred *cred)
 	int error;
 
 	f->f_cred = get_cred(cred);
+	f->f_owner.cred = get_cred(cred);
 	error = security_file_alloc(f);
 	if (unlikely(error)) {
 		put_cred(f->f_cred);
+		put_cred(f->f_owner.cred);
 		return error;
 	}
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 0283cf366c2a..345e8ff6d49a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -942,7 +942,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 *cred;/* cred of process setting the owner */
 	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 44488b1ab9a9..974bcc1c8f8f 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -196,7 +196,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 de3af33e6ff5..20357efa4a77 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -415,7 +415,6 @@ int security_file_mprotect(struct vm_area_struct *vma, unsigned long reqprot,
 			   unsigned long prot);
 int security_file_lock(struct file *file, unsigned int cmd);
 int security_file_fcntl(struct file *file, unsigned int cmd, unsigned long arg);
-void security_file_set_fowner(struct file *file);
 int security_file_send_sigiotask(struct task_struct *tsk,
 				 struct fown_struct *fown, int sig);
 int security_file_receive(struct file *file);
diff --git a/security/security.c b/security/security.c
index e5ca08789f74..34e7f5c86af5 100644
--- a/security/security.c
+++ b/security/security.c
@@ -2898,20 +2898,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 7eed331e90f0..a8f5ed66808d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -3644,8 +3644,6 @@ static int selinux_file_alloc_security(struct file *file)
 	u32 sid = current_sid();
 
 	fsec->sid = sid;
-	fsec->fown_sid = sid;
-
 	return 0;
 }
 
@@ -3918,33 +3916,20 @@ 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;
 	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);
-
-	fsec = selinux_file(file);
+	const struct task_security_struct *tsec =
+		selinux_cred(rcu_dereference(fown->cred));
 
 	if (!signum)
 		perm = signal_to_av(SIGIO); /* as per send_sigio_to_task */
 	else
 		perm = signal_to_av(signum);
 
-	return avc_has_perm(fsec->fown_sid, sid,
+	return avc_has_perm(tsec->sid, sid,
 			    SECCLASS_PROCESS, perm, NULL);
 }
 
@@ -7202,7 +7187,6 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(file_mprotect, selinux_file_mprotect),
 	LSM_HOOK_INIT(file_lock, selinux_file_lock),
 	LSM_HOOK_INIT(file_fcntl, selinux_file_fcntl),
-	LSM_HOOK_INIT(file_set_fowner, selinux_file_set_fowner),
 	LSM_HOOK_INIT(file_send_sigiotask, selinux_file_send_sigiotask),
 	LSM_HOOK_INIT(file_receive, selinux_file_receive),
 
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 f5cbec1e6a92..280a3da4c232 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -1650,26 +1650,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
@@ -1888,18 +1868,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
@@ -1914,7 +1882,6 @@ static void smack_file_set_fowner(struct file *file)
 static int smack_file_send_sigiotask(struct task_struct *tsk,
 				     struct fown_struct *fown, int signum)
 {
-	struct smack_known **blob;
 	struct smack_known *skp;
 	struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred));
 	const struct cred *tcred;
@@ -1928,8 +1895,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(smack_cred(rcu_dereference(fown->cred)));
 	rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
 	rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc);
 
@@ -5014,7 +4980,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 *),
@@ -5065,14 +5030,12 @@ static struct security_hook_list smack_hooks[] __ro_after_init = {
 	LSM_HOOK_INIT(inode_listsecurity, smack_inode_listsecurity),
 	LSM_HOOK_INIT(inode_getsecid, smack_inode_getsecid),
 
-	LSM_HOOK_INIT(file_alloc_security, smack_file_alloc_security),
 	LSM_HOOK_INIT(file_ioctl, smack_file_ioctl),
 	LSM_HOOK_INIT(file_ioctl_compat, smack_file_ioctl),
 	LSM_HOOK_INIT(file_lock, smack_file_lock),
 	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),
 
-- 
2.45.2

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Mickaël Salaün]

On Mon, Aug 12, 2024 at 03:09:08PM +0200, Jann Horn wrote:
> On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Fri, Aug 9, 2024 at 10:01 AM Jann Horn <jannh@google.com> wrote:
> > > On Fri, Aug 9, 2024 at 3:18 PM Mickaël Salaün <mic@digikod.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@digikod.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):

I just sent a quite similar patch just before syncing my emails...  The
main difference seems to be related to the initialization of the
f_owner's credentials.

> 
> 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.

> 

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Paul Moore]

On Mon, Aug 12, 2024 at 9:09 AM Jann Horn <jannh@google.com> wrote:
> On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:

...

> > 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):

I've very hesitant to drop the LSM hook from the F_SETOWN path both
because it is reasonable that other LSMs may want to do other things
here, and adding a LSM hook to the kernel, even if it is re-adding a
hook that was previously removed, is a difficult and painful process
with an uncertain outcome.

-- 
paul-moore.com

Re: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[Paul Moore]

On Mon, Aug 12, 2024 at 10:49 AM Mickaël Salaün <mic@digikod.net> wrote:
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index 44488b1ab9a9..974bcc1c8f8f 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -196,7 +196,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)

As I mentioned in the other thread, I don't want to see the
file_set_owner hook removed at this point in time.  I'm open to the
idea of moving it around, but as of right now I think it is important
to keep it around.

>  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)

-- 
paul-moore.com

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Jann Horn]

On Mon, Aug 12, 2024 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote:
> On Mon, Aug 12, 2024 at 9:09 AM Jann Horn <jannh@google.com> wrote:
> > On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
>
> ...
>
> > > 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):
>
> I've very hesitant to drop the LSM hook from the F_SETOWN path both
> because it is reasonable that other LSMs may want to do other things
> here,

What is an example for other things an LSM might want to do there? As
far as I understand, the whole point of this hook is to record the
identity of the sender of signals - are you talking about an LSM that
might not be storing credentials in struct cred, or something like
that?

> and adding a LSM hook to the kernel, even if it is re-adding a
> hook that was previously removed, is a difficult and painful process
> with an uncertain outcome.

Do you mean that even if the LSM hook ends up with zero users
remaining, you'd still want to keep it around in case it's needed
again later?

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Paul Moore]

On Mon, Aug 12, 2024 at 11:06 AM Jann Horn <jannh@google.com> wrote:
> On Mon, Aug 12, 2024 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Aug 12, 2024 at 9:09 AM Jann Horn <jannh@google.com> wrote:
> > > On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > ...
> >
> > > > 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):
> >
> > I've very hesitant to drop the LSM hook from the F_SETOWN path both
> > because it is reasonable that other LSMs may want to do other things
> > here,
>
> What is an example for other things an LSM might want to do there? As
> far as I understand, the whole point of this hook is to record the
> identity of the sender of signals - are you talking about an LSM that
> might not be storing credentials in struct cred, or something like
> that?

Sure.  The LSM framework is intentionally very vague and limited on
what restrictions it places on individual LSMs; we want to be able to
support a wide range of security models and concepts.  I view the
F_SETOWN hook are important because it is a control point that is used
to set/copy/transfer/whatever security attributes from the current
process to a file/fd for the purpose of managing signals on the fd.

> > and adding a LSM hook to the kernel, even if it is re-adding a
> > hook that was previously removed, is a difficult and painful process
> > with an uncertain outcome.
>
> Do you mean that even if the LSM hook ends up with zero users
> remaining, you'd still want to keep it around in case it's needed
> again later?

I want the security_file_set_fowner() hook to remain a viable hook for
capturing the current task's security attributes, regardless of what
security attributes the LSM is interested in capturing and where those
attributes are stored.

-- 
paul-moore.com

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Mickaël Salaün]

On Mon, Aug 12, 2024 at 12:30:03PM -0400, Paul Moore wrote:
> On Mon, Aug 12, 2024 at 11:06 AM Jann Horn <jannh@google.com> wrote:
> > On Mon, Aug 12, 2024 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Mon, Aug 12, 2024 at 9:09 AM Jann Horn <jannh@google.com> wrote:
> > > > On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > >
> > > ...
> > >
> > > > > 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):
> > >
> > > I've very hesitant to drop the LSM hook from the F_SETOWN path both
> > > because it is reasonable that other LSMs may want to do other things
> > > here,
> >
> > What is an example for other things an LSM might want to do there? As
> > far as I understand, the whole point of this hook is to record the
> > identity of the sender of signals - are you talking about an LSM that
> > might not be storing credentials in struct cred, or something like
> > that?
> 
> Sure.  The LSM framework is intentionally very vague and limited on
> what restrictions it places on individual LSMs; we want to be able to
> support a wide range of security models and concepts.  I view the
> F_SETOWN hook are important because it is a control point that is used
> to set/copy/transfer/whatever security attributes from the current
> process to a file/fd for the purpose of managing signals on the fd.
> 
> > > and adding a LSM hook to the kernel, even if it is re-adding a
> > > hook that was previously removed, is a difficult and painful process
> > > with an uncertain outcome.
> >
> > Do you mean that even if the LSM hook ends up with zero users
> > remaining, you'd still want to keep it around in case it's needed
> > again later?
> 
> I want the security_file_set_fowner() hook to remain a viable hook for
> capturing the current task's security attributes, regardless of what
> security attributes the LSM is interested in capturing and where those
> attributes are stored.

I don't see the point to keep an unused hook, we could add it back later
if there is a valid use case, but I'll send a v2 without this removal.

Re: f_modown and LSM inconsistency (was [PATCH v2 1/4] Landlock: Add signal control)

[Paul Moore]

On Mon, Aug 12, 2024 at 1:28 PM Mickaël Salaün <mic@digikod.net> wrote:
> On Mon, Aug 12, 2024 at 12:30:03PM -0400, Paul Moore wrote:
> > On Mon, Aug 12, 2024 at 11:06 AM Jann Horn <jannh@google.com> wrote:
> > > On Mon, Aug 12, 2024 at 4:57 PM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Mon, Aug 12, 2024 at 9:09 AM Jann Horn <jannh@google.com> wrote:
> > > > > On Mon, Aug 12, 2024 at 12:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > 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):
> > > >
> > > > I've very hesitant to drop the LSM hook from the F_SETOWN path both
> > > > because it is reasonable that other LSMs may want to do other things
> > > > here,
> > >
> > > What is an example for other things an LSM might want to do there? As
> > > far as I understand, the whole point of this hook is to record the
> > > identity of the sender of signals - are you talking about an LSM that
> > > might not be storing credentials in struct cred, or something like
> > > that?
> >
> > Sure.  The LSM framework is intentionally very vague and limited on
> > what restrictions it places on individual LSMs; we want to be able to
> > support a wide range of security models and concepts.  I view the
> > F_SETOWN hook are important because it is a control point that is used
> > to set/copy/transfer/whatever security attributes from the current
> > process to a file/fd for the purpose of managing signals on the fd.
> >
> > > > and adding a LSM hook to the kernel, even if it is re-adding a
> > > > hook that was previously removed, is a difficult and painful process
> > > > with an uncertain outcome.
> > >
> > > Do you mean that even if the LSM hook ends up with zero users
> > > remaining, you'd still want to keep it around in case it's needed
> > > again later?
> >
> > I want the security_file_set_fowner() hook to remain a viable hook for
> > capturing the current task's security attributes, regardless of what
> > security attributes the LSM is interested in capturing and where those
> > attributes are stored.
>
> I don't see the point to keep an unused hook, we could add it back later
> if there is a valid use case, but I'll send a v2 without this removal.

If it was simple to add LSM hooks, then I would agree, but history has
shown that not to be the case.

-- 
paul-moore.com

Re: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[kernel test robot]

Hi Mickaël,

kernel test robot noticed the following build errors:

[auto build test ERROR on pcmoore-selinux/next]
[also build test ERROR on linus/master v6.11-rc3 next-20240812]
[cannot apply to brauner-vfs/vfs.all]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/fs-security-Fix-file_set_fowner-LSM-hook-inconsistencies/20240813-004648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link:    https://lore.kernel.org/r/20240812144936.1616628-1-mic%40digikod.net
patch subject: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies
config: i386-buildonly-randconfig-002-20240813 (https://download.01.org/0day-ci/archive/20240813/202408130919.naHeqbVw-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240813/202408130919.naHeqbVw-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408130919.naHeqbVw-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/rculist.h:11,
                    from include/linux/dcache.h:8,
                    from include/linux/fs.h:8,
                    from include/uapi/linux/aio_abi.h:31,
                    from include/linux/syscalls.h:82,
                    from fs/fcntl.c:8:
   fs/fcntl.c: In function 'f_getowner_uids':
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/rcupdate.h:527:17: note: in definition of macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                 ^
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/rcupdate.h:527:38: note: in definition of macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                      ^
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
   In file included from <command-line>:
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:490:23: note: in definition of macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:9: note: in expansion of macro 'compiletime_assert'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |         ^~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:28: note: in expansion of macro '__native_word'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |                            ^~~~~~~~~~~~~
   include/asm-generic/rwonce.h:49:9: note: in expansion of macro 'compiletime_assert_rwonce_type'
      49 |         compiletime_assert_rwonce_type(x);                              \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:490:23: note: in definition of macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:9: note: in expansion of macro 'compiletime_assert'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |         ^~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:28: note: in expansion of macro '__native_word'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |                            ^~~~~~~~~~~~~
   include/asm-generic/rwonce.h:49:9: note: in expansion of macro 'compiletime_assert_rwonce_type'
      49 |         compiletime_assert_rwonce_type(x);                              \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:490:23: note: in definition of macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:9: note: in expansion of macro 'compiletime_assert'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |         ^~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:28: note: in expansion of macro '__native_word'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |                            ^~~~~~~~~~~~~
   include/asm-generic/rwonce.h:49:9: note: in expansion of macro 'compiletime_assert_rwonce_type'
      49 |         compiletime_assert_rwonce_type(x);                              \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:490:23: note: in definition of macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:9: note: in expansion of macro 'compiletime_assert'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |         ^~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:28: note: in expansion of macro '__native_word'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |                            ^~~~~~~~~~~~~
   include/asm-generic/rwonce.h:49:9: note: in expansion of macro 'compiletime_assert_rwonce_type'
      49 |         compiletime_assert_rwonce_type(x);                              \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:490:23: note: in definition of macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
   include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |         ^~~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:36:9: note: in expansion of macro 'compiletime_assert'
      36 |         compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
         |         ^~~~~~~~~~~~~~~~~~
   include/asm-generic/rwonce.h:49:9: note: in expansion of macro 'compiletime_assert_rwonce_type'
      49 |         compiletime_assert_rwonce_type(x);                              \
         |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/compiler_types.h:466:27: note: in definition of macro '__unqual_scalar_typeof'
     466 |                 _Generic((x),                                           \
         |                           ^
   include/asm-generic/rwonce.h:50:9: note: in expansion of macro '__READ_ONCE'
      50 |         __READ_ONCE(x);                                                 \
         |         ^~~~~~~~~~~
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
   In file included from ./arch/x86/include/generated/asm/rwonce.h:1,
                    from include/linux/compiler.h:305,
                    from include/linux/export.h:5,
                    from include/linux/linkage.h:7,
                    from include/linux/fs.h:5:
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/asm-generic/rwonce.h:44:73: note: in definition of macro '__READ_ONCE'
      44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                         ^
   include/linux/rcupdate.h:527:50: note: in expansion of macro 'READ_ONCE'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^~~~~~~~~
   include/linux/rcupdate.h:675:9: note: in expansion of macro '__rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^~~~~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~
>> fs/fcntl.c:250:50: error: invalid type argument of '->' (have 'struct fown_struct')
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                                  ^~
   include/linux/rcupdate.h:530:19: note: in definition of macro '__rcu_dereference_check'
     530 |         ((typeof(*p) __force __kernel *)(local)); \
         |                   ^
   include/linux/rcupdate.h:747:28: note: in expansion of macro 'rcu_dereference_check'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^~~~~~~~~~~~~~~~~~~~~
   fs/fcntl.c:250:21: note: in expansion of macro 'rcu_dereference'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^~~~~~~~~~~~~~~


vim +250 fs/fcntl.c

   239	
   240	#ifdef CONFIG_CHECKPOINT_RESTORE
   241	static int f_getowner_uids(struct file *filp, unsigned long arg)
   242	{
   243		struct user_namespace *user_ns = current_user_ns();
   244		uid_t __user *dst = (void __user *)arg;
   245		const struct cred *fown_cred;
   246		uid_t src[2];
   247		int err;
   248	
   249		rcu_read_lock();
 > 250		fown_cred = rcu_dereference(filp->f_owner->cred);
   251		src[0] = from_kuid(user_ns, fown_cred->uid);
   252		src[1] = from_kuid(user_ns, fown_cred->euid);
   253		rcu_read_unlock();
   254	
   255		err  = put_user(src[0], &dst[0]);
   256		err |= put_user(src[1], &dst[1]);
   257	
   258		return err;
   259	}
   260	#else
   261	static int f_getowner_uids(struct file *filp, unsigned long arg)
   262	{
   263		return -EINVAL;
   264	}
   265	#endif
   266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[kernel test robot]

Hi Mickaël,

kernel test robot noticed the following build errors:

[auto build test ERROR on pcmoore-selinux/next]
[also build test ERROR on linus/master v6.11-rc3 next-20240812]
[cannot apply to brauner-vfs/vfs.all]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/fs-security-Fix-file_set_fowner-LSM-hook-inconsistencies/20240813-004648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link:    https://lore.kernel.org/r/20240812144936.1616628-1-mic%40digikod.net
patch subject: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies
config: x86_64-buildonly-randconfig-003-20240813 (https://download.01.org/0day-ci/archive/20240813/202408130900.y6a7Si8X-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240813/202408130900.y6a7Si8X-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408130900.y6a7Si8X-lkp@intel.com/

All errors (new ones prefixed by >>):

>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:10: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                 ^
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:31: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                      ^
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                             ^~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |                              ^~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                             ^~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |                              ^~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                             ^~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |                              ^~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   note: (skipping 3 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                             ^~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |                              ^~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
   include/linux/compiler_types.h:510:22: note: expanded from macro 'compiletime_assert'
     510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
         |                             ^~~~~~~~~
   include/linux/compiler_types.h:498:23: note: expanded from macro '_compiletime_assert'
     498 |         __compiletime_assert(condition, msg, prefix, suffix)
         |                              ^~~~~~~~~
   include/linux/compiler_types.h:490:9: note: expanded from macro '__compiletime_assert'
     490 |                 if (!(condition))                                       \
         |                       ^~~~~~~~~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   include/asm-generic/rwonce.h:50:14: note: expanded from macro 'READ_ONCE'
      50 |         __READ_ONCE(x);                                                 \
         |                     ^
   include/asm-generic/rwonce.h:44:65: note: expanded from macro '__READ_ONCE'
      44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                  ^
   include/linux/compiler_types.h:466:13: note: expanded from macro '__unqual_scalar_typeof'
     466 |                 _Generic((x),                                           \
         |                           ^
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   include/asm-generic/rwonce.h:50:14: note: expanded from macro 'READ_ONCE'
      50 |         __READ_ONCE(x);                                                 \
         |                     ^
   include/asm-generic/rwonce.h:44:65: note: expanded from macro '__READ_ONCE'
      44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                  ^
   include/linux/compiler_types.h:473:15: note: expanded from macro '__unqual_scalar_typeof'
     473 |                          default: (x)))
         |                                    ^
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:527:53: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                            ^
   include/asm-generic/rwonce.h:50:14: note: expanded from macro 'READ_ONCE'
      50 |         __READ_ONCE(x);                                                 \
         |                     ^
   include/asm-generic/rwonce.h:44:72: note: expanded from macro '__READ_ONCE'
      44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                         ^
>> fs/fcntl.c:250:14: error: cannot take the address of an rvalue of type 'const struct cred *'
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                     ^               ~~~~~~~~~~~~~~~~~~~
   include/linux/rcupdate.h:747:28: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                            ^                     ~
   include/linux/rcupdate.h:675:2: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |         ^                        ~
   include/linux/rcupdate.h:527:43: note: expanded from macro '__rcu_dereference_check'
     527 |         typeof(*p) *local = (typeof(*p) *__force)READ_ONCE(p); \
         |                                                  ^         ~
   include/asm-generic/rwonce.h:50:2: note: expanded from macro 'READ_ONCE'
      50 |         __READ_ONCE(x);                                                 \
         |         ^           ~
   include/asm-generic/rwonce.h:44:70: note: expanded from macro '__READ_ONCE'
      44 | #define __READ_ONCE(x)  (*(const volatile __unqual_scalar_typeof(x) *)&(x))
         |                                                                       ^ ~
>> fs/fcntl.c:250:43: error: member reference type 'struct fown_struct' is not a pointer; did you mean to use '.'?
     250 |         fown_cred = rcu_dereference(filp->f_owner->cred);
         |                                     ~~~~~~~~~~~~~^~
         |                                                  .
   include/linux/rcupdate.h:747:50: note: expanded from macro 'rcu_dereference'
     747 | #define rcu_dereference(p) rcu_dereference_check(p, 0)
         |                                                  ^
   include/linux/rcupdate.h:675:27: note: expanded from macro 'rcu_dereference_check'
     675 |         __rcu_dereference_check((p), __UNIQUE_ID(rcu), \
         |                                  ^
   include/linux/rcupdate.h:530:12: note: expanded from macro '__rcu_dereference_check'
     530 |         ((typeof(*p) __force __kernel *)(local)); \
         |                   ^
   12 errors generated.


vim +250 fs/fcntl.c

   239	
   240	#ifdef CONFIG_CHECKPOINT_RESTORE
   241	static int f_getowner_uids(struct file *filp, unsigned long arg)
   242	{
   243		struct user_namespace *user_ns = current_user_ns();
   244		uid_t __user *dst = (void __user *)arg;
   245		const struct cred *fown_cred;
   246		uid_t src[2];
   247		int err;
   248	
   249		rcu_read_lock();
 > 250		fown_cred = rcu_dereference(filp->f_owner->cred);
   251		src[0] = from_kuid(user_ns, fown_cred->uid);
   252		src[1] = from_kuid(user_ns, fown_cred->euid);
   253		rcu_read_unlock();
   254	
   255		err  = put_user(src[0], &dst[0]);
   256		err |= put_user(src[1], &dst[1]);
   257	
   258		return err;
   259	}
   260	#else
   261	static int f_getowner_uids(struct file *filp, unsigned long arg)
   262	{
   263		return -EINVAL;
   264	}
   265	#endif
   266	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[kernel test robot]

Hi Mickaël,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on linus/master v6.11-rc3 next-20240812]
[cannot apply to brauner-vfs/vfs.all]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/fs-security-Fix-file_set_fowner-LSM-hook-inconsistencies/20240813-004648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link:    https://lore.kernel.org/r/20240812144936.1616628-1-mic%40digikod.net
patch subject: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20240813/202408130946.6oMWnayg-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240813/202408130946.6oMWnayg-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408130946.6oMWnayg-lkp@intel.com/

All warnings (new ones prefixed by >>):

   security/smack/smack_lsm.c: In function 'smack_file_send_sigiotask':
>> security/smack/smack_lsm.c:1913:22: warning: variable 'file' set but not used [-Wunused-but-set-variable]
    1913 |         struct file *file;
         |                      ^~~~


vim +/file +1913 security/smack/smack_lsm.c

7898e1f8e9eb1b Casey Schaufler 2011-01-17  1895  
e114e473771c84 Casey Schaufler 2008-02-04  1896  /**
e114e473771c84 Casey Schaufler 2008-02-04  1897   * smack_file_send_sigiotask - Smack on sigio
e114e473771c84 Casey Schaufler 2008-02-04  1898   * @tsk: The target task
e114e473771c84 Casey Schaufler 2008-02-04  1899   * @fown: the object the signal come from
e114e473771c84 Casey Schaufler 2008-02-04  1900   * @signum: unused
e114e473771c84 Casey Schaufler 2008-02-04  1901   *
e114e473771c84 Casey Schaufler 2008-02-04  1902   * Allow a privileged task to get signals even if it shouldn't
e114e473771c84 Casey Schaufler 2008-02-04  1903   *
e114e473771c84 Casey Schaufler 2008-02-04  1904   * Returns 0 if a subject with the object's smack could
e114e473771c84 Casey Schaufler 2008-02-04  1905   * write to the task, an error code otherwise.
e114e473771c84 Casey Schaufler 2008-02-04  1906   */
e114e473771c84 Casey Schaufler 2008-02-04  1907  static int smack_file_send_sigiotask(struct task_struct *tsk,
e114e473771c84 Casey Schaufler 2008-02-04  1908  				     struct fown_struct *fown, int signum)
e114e473771c84 Casey Schaufler 2008-02-04  1909  {
2f823ff8bec03a Casey Schaufler 2013-05-22  1910  	struct smack_known *skp;
b17103a8b8ae9c Casey Schaufler 2018-11-09  1911  	struct smack_known *tkp = smk_of_task(smack_cred(tsk->cred));
dcb569cf6ac99c Casey Schaufler 2018-09-18  1912  	const struct cred *tcred;
e114e473771c84 Casey Schaufler 2008-02-04 @1913  	struct file *file;
e114e473771c84 Casey Schaufler 2008-02-04  1914  	int rc;
ecfcc53fef3c35 Etienne Basset  2009-04-08  1915  	struct smk_audit_info ad;
e114e473771c84 Casey Schaufler 2008-02-04  1916  
e114e473771c84 Casey Schaufler 2008-02-04  1917  	/*
e114e473771c84 Casey Schaufler 2008-02-04  1918  	 * struct fown_struct is never outside the context of a struct file
e114e473771c84 Casey Schaufler 2008-02-04  1919  	 */
e114e473771c84 Casey Schaufler 2008-02-04  1920  	file = container_of(fown, struct file, f_owner);
7898e1f8e9eb1b Casey Schaufler 2011-01-17  1921  
ecfcc53fef3c35 Etienne Basset  2009-04-08  1922  	/* we don't log here as rc can be overriden */
32192c8e14ea34 Mickaël Salaün  2024-08-12  1923  	skp = smk_of_task(smack_cred(rcu_dereference(fown->cred)));
c60b906673eebb Casey Schaufler 2016-08-30  1924  	rc = smk_access(skp, tkp, MAY_DELIVER, NULL);
c60b906673eebb Casey Schaufler 2016-08-30  1925  	rc = smk_bu_note("sigiotask", skp, tkp, MAY_DELIVER, rc);
dcb569cf6ac99c Casey Schaufler 2018-09-18  1926  
dcb569cf6ac99c Casey Schaufler 2018-09-18  1927  	rcu_read_lock();
dcb569cf6ac99c Casey Schaufler 2018-09-18  1928  	tcred = __task_cred(tsk);
dcb569cf6ac99c Casey Schaufler 2018-09-18  1929  	if (rc != 0 && smack_privileged_cred(CAP_MAC_OVERRIDE, tcred))
ecfcc53fef3c35 Etienne Basset  2009-04-08  1930  		rc = 0;
dcb569cf6ac99c Casey Schaufler 2018-09-18  1931  	rcu_read_unlock();
ecfcc53fef3c35 Etienne Basset  2009-04-08  1932  
ecfcc53fef3c35 Etienne Basset  2009-04-08  1933  	smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK);
ecfcc53fef3c35 Etienne Basset  2009-04-08  1934  	smk_ad_setfield_u_tsk(&ad, tsk);
c60b906673eebb Casey Schaufler 2016-08-30  1935  	smack_log(skp->smk_known, tkp->smk_known, MAY_DELIVER, rc, &ad);
e114e473771c84 Casey Schaufler 2008-02-04  1936  	return rc;
e114e473771c84 Casey Schaufler 2008-02-04  1937  }
e114e473771c84 Casey Schaufler 2008-02-04  1938  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies

[kernel test robot]

Hi Mickaël,

kernel test robot noticed the following build warnings:

[auto build test WARNING on pcmoore-selinux/next]
[also build test WARNING on linus/master v6.11-rc3]
[cannot apply to brauner-vfs/vfs.all next-20240813]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Micka-l-Sala-n/fs-security-Fix-file_set_fowner-LSM-hook-inconsistencies/20240813-004648
base:   https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/selinux.git next
patch link:    https://lore.kernel.org/r/20240812144936.1616628-1-mic%40digikod.net
patch subject: [PATCH] fs,security: Fix file_set_fowner LSM hook inconsistencies
config: x86_64-randconfig-122-20240813 (https://download.01.org/0day-ci/archive/20240813/202408131900.xhbYFHR4-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-12) 11.3.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240813/202408131900.xhbYFHR4-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202408131900.xhbYFHR4-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> fs/file_table.c:153:25: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected struct cred const [noderef] __rcu *cred @@     got struct cred const * @@
   fs/file_table.c:153:25: sparse:     expected struct cred const [noderef] __rcu *cred
   fs/file_table.c:153:25: sparse:     got struct cred const *
>> fs/file_table.c:157:36: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct cred const *cred @@     got struct cred const [noderef] __rcu *cred @@
   fs/file_table.c:157:36: sparse:     expected struct cred const *cred
   fs/file_table.c:157:36: sparse:     got struct cred const [noderef] __rcu *cred
   fs/file_table.c:69:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct cred const *cred @@     got struct cred const [noderef] __rcu *cred @@
   fs/file_table.c:69:28: sparse:     expected struct cred const *cred
   fs/file_table.c:69:28: sparse:     got struct cred const [noderef] __rcu *cred
   fs/file_table.c:69:28: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected struct cred const *cred @@     got struct cred const [noderef] __rcu *cred @@
   fs/file_table.c:69:28: sparse:     expected struct cred const *cred
   fs/file_table.c:69:28: sparse:     got struct cred const [noderef] __rcu *cred

vim +153 fs/file_table.c

   147	
   148	static int init_file(struct file *f, int flags, const struct cred *cred)
   149	{
   150		int error;
   151	
   152		f->f_cred = get_cred(cred);
 > 153		f->f_owner.cred = get_cred(cred);
   154		error = security_file_alloc(f);
   155		if (unlikely(error)) {
   156			put_cred(f->f_cred);
 > 157			put_cred(f->f_owner.cred);
   158			return error;
   159		}
   160	
   161		rwlock_init(&f->f_owner.lock);
   162		spin_lock_init(&f->f_lock);
   163		mutex_init(&f->f_pos_lock);
   164		f->f_flags = flags;
   165		f->f_mode = OPEN_FMODE(flags);
   166		/* f->f_version: 0 */
   167	
   168		/*
   169		 * We're SLAB_TYPESAFE_BY_RCU so initialize f_count last. While
   170		 * fget-rcu pattern users need to be able to handle spurious
   171		 * refcount bumps we should reinitialize the reused file first.
   172		 */
   173		atomic_long_set(&f->f_count, 1);
   174		return 0;
   175	}
   176	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki