linux-kernel - Re: [PATCH v2 3/3] Wire UFFD up to SELinux

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 25 Mar 2020 16:49:14 -0700
From:   Casey Schaufler <casey@...aufler-ca.com>
To:     Daniel Colascione <dancol@...gle.com>, timmurray@...gle.com,
        selinux@...r.kernel.org, linux-security-module@...r.kernel.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org, viro@...iv.linux.org.uk, paul@...l-moore.com,
        nnk@...gle.com, sds@...ho.nsa.gov, lokeshgidra@...gle.com,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v2 3/3] Wire UFFD up to SELinux

On 3/25/2020 4:02 PM, Daniel Colascione wrote:
> This change gives userfaultfd file descriptors a real security
> context, allowing policy to act on them.

You should change the title to "Wire UFFD up to secure anon inodes".
This code should support any LSM that wants to control anon inodes.
If it doesn't, it isn't correct.

All references to SELinux behavior (i.e. assigning a "security context")
should be restricted to the SELinux specific bits of the patch set. You
should be describing how any LSM can use this, not just the LSM you've
targeted.

>
> Signed-off-by: Daniel Colascione <dancol@...gle.com>
> ---
>  fs/userfaultfd.c | 34 +++++++++++++++++++++++++++++-----
>  1 file changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 07b0f6e03849..78ff5d898733 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -76,6 +76,8 @@ struct userfaultfd_ctx {
>  	bool mmap_changing;
>  	/* mm with one ore more vmas attached to this userfaultfd_ctx */
>  	struct mm_struct *mm;
> +	/* The inode that owns this context --- not a strong reference.  */
> +	const struct inode *owner;
>  };
>  
>  struct userfaultfd_fork_ctx {
> @@ -1014,14 +1016,18 @@ static __poll_t userfaultfd_poll(struct file *file, poll_table *wait)
>  	}
>  }
>  
> +static const struct file_operations userfaultfd_fops;
> +
>  static int resolve_userfault_fork(struct userfaultfd_ctx *ctx,
>  				  struct userfaultfd_ctx *new,
>  				  struct uffd_msg *msg)
>  {
>  	int fd;
>  
> -	fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, new,
> -			      O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS));
> +	fd = anon_inode_getfd_secure(
> +		"[userfaultfd]", &userfaultfd_fops, new,
> +		O_RDWR | (new->flags & UFFD_SHARED_FCNTL_FLAGS),
> +		ctx->owner);
>  	if (fd < 0)
>  		return fd;
>  
> @@ -1918,7 +1924,7 @@ static void userfaultfd_show_fdinfo(struct seq_file *m, struct file *f)
>  }
>  #endif
>  
> -const struct file_operations userfaultfd_fops = {
> +static const struct file_operations userfaultfd_fops = {
>  #ifdef CONFIG_PROC_FS
>  	.show_fdinfo	= userfaultfd_show_fdinfo,
>  #endif
> @@ -1943,6 +1949,7 @@ static void init_once_userfaultfd_ctx(void *mem)
>  
>  SYSCALL_DEFINE1(userfaultfd, int, flags)
>  {
> +	struct file *file;
>  	struct userfaultfd_ctx *ctx;
>  	int fd;
>  
> @@ -1972,8 +1979,25 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
>  	/* prevent the mm struct to be freed */
>  	mmgrab(ctx->mm);
>  
> -	fd = anon_inode_getfd("[userfaultfd]", &userfaultfd_fops, ctx,
> -			      O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS));
> +	file = anon_inode_getfile_secure(
> +		"[userfaultfd]", &userfaultfd_fops, ctx,
> +		O_RDWR | (flags & UFFD_SHARED_FCNTL_FLAGS),
> +		NULL);
> +	if (IS_ERR(file)) {
> +		fd = PTR_ERR(file);
> +		goto out;
> +	}
> +
> +	fd = get_unused_fd_flags(O_RDONLY | O_CLOEXEC);
> +	if (fd < 0) {
> +		fput(file);
> +		goto out;
> +	}
> +
> +	ctx->owner = file_inode(file);
> +	fd_install(fd, file);
> +
> +out:
>  	if (fd < 0) {
>  		mmdrop(ctx->mm);
>  		kmem_cache_free(userfaultfd_ctx_cachep, ctx);