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