Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Wenwen Wang <wang6495@umn.edu>
To:     Wenwen Wang <wang6495@umn.edu>
Cc:     Kangjie Lu <kjlu@umn.edu>, Hans de Goede <hdegoede@redhat.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-kernel@vger.kernel.org (open list)
Subject: [PATCH] virt: vbox: fix a missing-check bug
Date:   Sat,  5 May 2018 22:30:48 -0500
Message-Id: <1525577448-16071-1-git-send-email-wang6495@umn.edu>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@umn.edu>
---
 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;
 
-- 
2.7.4