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

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

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.

> 

Powered by blists - more mailing lists