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