On Tue, May 8, 2018 at 6:46 AM, Hans de Goede <[email protected]> wrote: > 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 <[email protected]> > > > 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 <[email protected]> > 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 <[email protected]> > Signed-off-by: Hans de Goede <[email protected]> > --- > 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?
Thanks for your response. Yes, this fix also works. Wenwen
