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: [email protected]
> Signed-off-by: Christian Brauner (Amutable) <[email protected]>
Looks good. Feel free to add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
