[PATCH RFC 33/38] proc: sysctl: copy to and from userspace without holding the table

[Jan Kara]

On Mon 02-11-26 15:21:48, Christian Brauner wrote:
> proc_sys_call_handler() grabs the table, copies in what is written,
> calls the handler and copies what it produced out to userspace before
> it lets go of the table again. Unregistering a table waits until nobody
> uses it anymore. So a reader or writer whose copy faults for as long as
> it wants, with userfaultfd or a stalled FUSE mapping, keeps
> unregister_sysctl_table() waiting, and its callers like to hold locks
> while they do that. addrconf unregisters the ipv6 sysctls of a device
> with RTNL held, so a stalled read of /proc/sys/net/ipv6/conf/<dev>/mtu
> holds up every RTNL user until the reader gives up.
> 
> Copy in before grabbing the table and copy out after letting go of it,
> like procfs and kernfs do for their files. The buffer is ours, nothing
> needs the table for the copies.
> 
> That moves the allocation and the copy in before the permission check.
> A write that can't be copied in, or a request that is too large, now
> fails with -EFAULT or -ENOMEM where it used to fail with -EPERM.
> 
> Fixes: 330d57fb98a9 ("[PATCH] Fix sysctl unregistration oops (CVE-2005-2709)")
> Cc: stable at vger.kernel.org
> Signed-off-by: Christian Brauner (Amutable) <brauner at kernel.org>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack at suse.cz>

								Honza

> ---
>  fs/proc/proc_sysctl.c | 50 ++++++++++++++++++++++++++++----------------------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/proc/proc_sysctl.c b/fs/proc/proc_sysctl.c
> index 38b61887b59c..2150cf842919 100644
> --- a/fs/proc/proc_sysctl.c
> +++ b/fs/proc/proc_sysctl.c
> @@ -602,14 +602,36 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>  		int write)
>  {
>  	struct inode *inode = file_inode(iocb->ki_filp);
> -	struct ctl_table_header *head = grab_header(inode);
>  	const struct ctl_table *table = PROC_I(inode)->sysctl_entry;
> +	struct ctl_table_header *head;
>  	size_t count = iov_iter_count(iter);
>  	char *kbuf;
>  	ssize_t error;
>  
> -	if (IS_ERR(head))
> -		return PTR_ERR(head);
> +	/* don't even try if the size is too large */
> +	if (count >= KMALLOC_MAX_SIZE)
> +		return -ENOMEM;
> +	kbuf = kvzalloc(count + 1, GFP_KERNEL);
> +	if (!kbuf)
> +		return -ENOMEM;
> +
> +	/*
> +	 * Copy in before grabbing the table and copy out after letting go of
> +	 * it. Unregistering the table waits for us and a copy that faults can
> +	 * take as long as userspace wants.
> +	 */
> +	if (write) {
> +		error = -EFAULT;
> +		if (!copy_from_iter_full(kbuf, count, iter))
> +			goto out_free_buf;
> +		kbuf[count] = '\0';
> +	}
> +
> +	head = grab_header(inode);
> +	if (IS_ERR(head)) {
> +		error = PTR_ERR(head);
> +		goto out_free_buf;
> +	}
>  
>  	/*
>  	 * At this point we know that the sysctl was not unregistered
> @@ -624,28 +646,15 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>  	if (!table->proc_handler)
>  		goto out;
>  
> -	/* don't even try if the size is too large */
> -	error = -ENOMEM;
> -	if (count >= KMALLOC_MAX_SIZE)
> -		goto out;
> -	kbuf = kvzalloc(count + 1, GFP_KERNEL);
> -	if (!kbuf)
> -		goto out;
> -
> -	if (write) {
> -		error = -EFAULT;
> -		if (!copy_from_iter_full(kbuf, count, iter))
> -			goto out_free_buf;
> -		kbuf[count] = '\0';
> -	}
> -
>  	error = BPF_CGROUP_RUN_PROG_SYSCTL(head, table, write, &kbuf, &count,
>  					   &iocb->ki_pos);
>  	if (error)
> -		goto out_free_buf;
> +		goto out;
>  
>  	/* careful: calling conventions are nasty here */
>  	error = table->proc_handler(table, write, kbuf, &count, &iocb->ki_pos);
> +out:
> +	sysctl_head_finish(head);
>  	if (error)
>  		goto out_free_buf;
>  
> @@ -658,9 +667,6 @@ static ssize_t proc_sys_call_handler(struct kiocb *iocb, struct iov_iter *iter,
>  	error = count;
>  out_free_buf:
>  	kvfree(kbuf);
> -out:
> -	sysctl_head_finish(head);
> -
>  	return error;
>  }
>  
> 
> -- 
> 2.53.0
> 
-- 
Jan Kara <jack at suse.com>
SUSE Labs, CR