LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] virt: vbox: Only copy_from_user the request-header once
@ 2018-05-08 13:46 Hans de Goede
  2018-05-08 14:07 ` Hans de Goede
  2018-05-08 14:21 ` Greg Kroah-Hartman
  0 siblings, 2 replies; 4+ messages in thread
From: Hans de Goede @ 2018-05-08 13:46 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman; +Cc: Hans de Goede, linux-kernel

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@umn.edu>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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;
 	}
-- 
2.17.0

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] virt: vbox: Only copy_from_user the request-header once
  2018-05-08 13:46 [PATCH] virt: vbox: Only copy_from_user the request-header once Hans de Goede
@ 2018-05-08 14:07 ` Hans de Goede
  2018-05-08 14:21 ` Greg Kroah-Hartman
  1 sibling, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2018-05-08 14:07 UTC (permalink / raw)
  To: Arnd Bergmann, Greg Kroah-Hartman; +Cc: linux-kernel

Hi,

On 08-05-18 15:46, Hans de Goede 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.
> 
> 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@umn.edu>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Wenwen just send a v2 of his patch, lets go with his (identical)
version as he has done the hard work of tracking this down.

Regards,

Hans




> ---
>   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;
>   	}
> 

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] virt: vbox: Only copy_from_user the request-header once
  2018-05-08 13:46 [PATCH] virt: vbox: Only copy_from_user the request-header once Hans de Goede
  2018-05-08 14:07 ` Hans de Goede
@ 2018-05-08 14:21 ` Greg Kroah-Hartman
  2018-05-08 14:52   ` Hans de Goede
  1 sibling, 1 reply; 4+ messages in thread
From: Greg Kroah-Hartman @ 2018-05-08 14:21 UTC (permalink / raw)
  To: Hans de Goede; +Cc: Arnd Bergmann, linux-kernel

On Tue, May 08, 2018 at 03:46:59PM +0200, Hans de Goede 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.
> 
> 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@umn.edu>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Does this need to go into 4.17-final?  Any older kernels?  Or can it
wait for 4.18-rc1?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH] virt: vbox: Only copy_from_user the request-header once
  2018-05-08 14:21 ` Greg Kroah-Hartman
@ 2018-05-08 14:52   ` Hans de Goede
  0 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2018-05-08 14:52 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: Arnd Bergmann, linux-kernel

Hi,

On 08-05-18 16:21, Greg Kroah-Hartman wrote:
> On Tue, May 08, 2018 at 03:46:59PM +0200, Hans de Goede 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.
>>
>> 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@umn.edu>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Does this need to go into 4.17-final?  Any older kernels?  Or can it
> wait for 4.18-rc1?

I believe this can wait for 4.18-rc1.

Regards,

Hans

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2018-05-08 14:52 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-08 13:46 [PATCH] virt: vbox: Only copy_from_user the request-header once Hans de Goede
2018-05-08 14:07 ` Hans de Goede
2018-05-08 14:21 ` Greg Kroah-Hartman
2018-05-08 14:52   ` Hans de Goede

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).