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.
