Re: [PATCH] virt: vbox: fix a missing-check bug

[Hans de Goede]

Hi Wenwen,

On 06-05-18 05:30, Wenwen Wang wrote:
> In vbg_misc_device_ioctl(), the header of the ioctl argument is copied from
> the userspace pointer 'arg' and saved to the kernel object 'hdr'. Then the
> 'version', 'size_in', and 'size_out' fields of 'hdr' are verified. For
> example, if 'hdr.version' is not VBG_IOCTL_HDR_VERSION, an error code
> -EINVAL will be returned. If 'hdr' can pass all verifications, the whole
> structure of the ioctl argument is copied once again from 'arg' and saved
> to 'buf'. Then the function vbg_core_ioctl() is invoked to execute the
> ioctl command. Given that the 'arg' pointer resides in userspace, a
> malicious userspace process can race to change the data pointed to by 'arg'
> between the two copies. By doing so, the user can bypass the verifications
> on the ioctl argument, which can cause vbg_core_ioctl() to work on unsecure
> data because it assumes the 'version', 'size_in', and 'size_out' have been
> verified by vbg_misc_device_ioctl(), as mentioned in the comment in
> vbg_core_ioctl():
>
>          /*
>           * hdr->version and hdr->size_in / hdr->size_out minimum size are
>           * already checked by vbg_misc_device_ioctl().
>           */
>
> This patch adds checks after the second copy to ensure the consistency
> between the data obtained in the two copies. In case an inconsistency is
> detected, an error code -EINVAL will be returned.
>
> Signed-off-by: Wenwen Wang <wang6495@xxxxxxx>

Thank you for finding this. I don't think that doing a second check is
a good solution, by copy and pasting the checks we run the risk that
any future additional checks are omitted from one copy of the checks.

Instead I think we should simply avoid the 2nd copy of the header, like this:

From 0c50b0dce3cf25a0ee9794c5816d9a0232d29e0a Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
Date: Tue, 8 May 2018 13:23:01 +0200
Subject: [PATCH 3/3] virt: vbox: Only copy_from_user the request-header once

In vbg_misc_device_ioctl(), the header of the ioctl argument is copied from
the userspace pointer 'arg' and saved to the kernel object 'hdr'. Then the
'version', 'size_in', and 'size_out' fields of 'hdr' are verified.

Before this commit, after the checks a buffer for the entire request would
be allocated and then all data including the verified header would be
copied from the userspace 'arg' pointer again.

Given that the 'arg' pointer resides in userspace, a malicious userspace
process can race to change the data pointed to by 'arg' between the two
copies. By doing so, the user can bypass the verifications on the ioctl
argument.

This commit fixes this by using the already checked copy of the header
to fill the header part of the allocated buffer and only copying the
remainder of the data from userspace.

Reported-by: Wenwen Wang <wang6495@xxxxxxx>
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
 drivers/virt/vboxguest/vboxguest_linux.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/virt/vboxguest/vboxguest_linux.c b/drivers/virt/vboxguest/vboxguest_linux.c
index 398d22693234..6e2a9619192d 100644
--- a/drivers/virt/vboxguest/vboxguest_linux.c
+++ b/drivers/virt/vboxguest/vboxguest_linux.c
@@ -121,7 +121,9 @@ static long vbg_misc_device_ioctl(struct file *filp, unsigned int req,
 	if (!buf)
 		return -ENOMEM;

-	if (copy_from_user(buf, (void *)arg, hdr.size_in)) {
+	*((struct vbg_ioctl_hdr *)buf) = hdr;
+	if (copy_from_user(buf + sizeof(hdr), (void *)arg + sizeof(hdr),
+			   hdr.size_in - sizeof(hdr))) {
 		ret = -EFAULT;
 		goto out;
 	}

Do you agree that this would also fix the race window you found?

Regards,

Hans




> ---
>   drivers/virt/vboxguest/vboxguest_linux.c | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/virt/vboxguest/vboxguest_linux.c b/drivers/virt/vboxguest/vboxguest_linux.c
> index 398d226..6b525259 100644
> --- a/drivers/virt/vboxguest/vboxguest_linux.c
> +++ b/drivers/virt/vboxguest/vboxguest_linux.c
> @@ -125,6 +125,12 @@ static long vbg_misc_device_ioctl(struct file *filp, unsigned int req,
>   		ret = -EFAULT;
>   		goto out;
>   	}
> +	if (((struct vbg_ioctl_hdr *)buf)->version != hdr.version ||
> +	    ((struct vbg_ioctl_hdr *)buf)->size_in != hdr.size_in ||
> +	    ((struct vbg_ioctl_hdr *)buf)->size_out != hdr.size_out) {
> +		ret = -EINVAL;
> +		goto out;
> +	}
>   	if (hdr.size_in < size)
>   		memset(buf + hdr.size_in, 0, size -  hdr.size_in);
>   
> @@ -133,11 +139,6 @@ static long vbg_misc_device_ioctl(struct file *filp, unsigned int req,
>   		goto out;
>   
>   	returned_size = ((struct vbg_ioctl_hdr *)buf)->size_out;
> -	if (returned_size > size) {
> -		vbg_debug("%s: too much output data %zu > %zu\n",
> -			  __func__, returned_size, size);
> -		returned_size = size;
> -	}
>   	if (copy_to_user((void *)arg, buf, returned_size) != 0)
>   		ret = -EFAULT;
>