Messages in this thread Patch in this message | | | From | Wenwen Wang <> | Subject | [PATCH] virt: vbox: fix a missing-check bug | Date | Sat, 5 May 2018 22:30:48 -0500 |
| |
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
| |