[Qemu-devel] [PATCH] usb: fix up post load checks

[Qemu-devel] [PATCH] usb: fix up post load checks

[Michael S. Tsirkin]

Correct post load checks:
1. dev->setup_len == sizeof(dev->data_buf)
    seems fine, no need to fail migration
2. When state is DATA, passing index > len
   will cause memcpy with negative length,
   resulting in heap overflow

First of the issues was reported by dgilbert.

Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 hw/usb/bus.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/usb/bus.c b/hw/usb/bus.c
index e48b19f..2721719 100644
--- a/hw/usb/bus.c
+++ b/hw/usb/bus.c
@@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int version_id)
     }
     if (dev->setup_index < 0 ||
         dev->setup_len < 0 ||
-        dev->setup_index >= sizeof(dev->data_buf) ||
-        dev->setup_len >= sizeof(dev->data_buf)) {
+        (dev->setup_state == SETUP_STATE_DATA &&
+         dev->setup_index > dev->setup_len) ||
+        dev->setup_len > sizeof(dev->data_buf)) {
         return -EINVAL;
     }
     return 0;
-- 
MST

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Gonglei (Arei)]

Hi,

> -----Original Message-----
> From: qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org
> [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On
> Behalf Of Michael S. Tsirkin
> Sent: Monday, May 12, 2014 8:16 PM
> To: qemu-devel@nongnu.org
> Cc: Gerd Hoffmann; dgilbert@redhat.com
> Subject: [Qemu-devel] [PATCH] usb: fix up post load checks
> 
> Correct post load checks:
> 1. dev->setup_len == sizeof(dev->data_buf)
>     seems fine, no need to fail migration
> 2. When state is DATA, passing index > len
>    will cause memcpy with negative length,
>    resulting in heap overflow
> 
> First of the issues was reported by dgilbert.
> 
> Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  hw/usb/bus.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index e48b19f..2721719 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int
> version_id)
>      }
>      if (dev->setup_index < 0 ||
>          dev->setup_len < 0 ||
> -        dev->setup_index >= sizeof(dev->data_buf) ||

Does this check should be deleted ?

> -        dev->setup_len >= sizeof(dev->data_buf)) {
> +        (dev->setup_state == SETUP_STATE_DATA &&
> +         dev->setup_index > dev->setup_len) ||
> +        dev->setup_len > sizeof(dev->data_buf)) {
>          return -EINVAL;
>      }
>      return 0;
> --
> MST

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Gerd Hoffmann]

On Di, 2014-05-13 at 03:02 +0000, Gonglei (Arei) wrote:
> Hi,
> 
> > -----Original Message-----
> > From: qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org
> > [mailto:qemu-devel-bounces+arei.gonglei=huawei.com@nongnu.org] On
> > Behalf Of Michael S. Tsirkin
> > Sent: Monday, May 12, 2014 8:16 PM
> > To: qemu-devel@nongnu.org
> > Cc: Gerd Hoffmann; dgilbert@redhat.com
> > Subject: [Qemu-devel] [PATCH] usb: fix up post load checks
> > 
> > Correct post load checks:
> > 1. dev->setup_len == sizeof(dev->data_buf)
> >     seems fine, no need to fail migration
> > 2. When state is DATA, passing index > len
> >    will cause memcpy with negative length,
> >    resulting in heap overflow
> > 
> > First of the issues was reported by dgilbert.
> > 
> > Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  hw/usb/bus.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> > index e48b19f..2721719 100644
> > --- a/hw/usb/bus.c
> > +++ b/hw/usb/bus.c
> > @@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int
> > version_id)
> >      }
> >      if (dev->setup_index < 0 ||
> >          dev->setup_len < 0 ||
> > -        dev->setup_index >= sizeof(dev->data_buf) ||
> 
> Does this check should be deleted ?

It's ok, index <= len && len <= sizeof(buf) implies index <= sizeof(buf)

> 
> > -        dev->setup_len >= sizeof(dev->data_buf)) {
> > +        (dev->setup_state == SETUP_STATE_DATA &&
> > +         dev->setup_index > dev->setup_len) ||
> > +        dev->setup_len > sizeof(dev->data_buf)) {
> >          return -EINVAL;
> >      }
> >      return 0;
> > --
> > MST
> 
> Best regards,
> -Gonglei

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Gerd Hoffmann]

  Hi,

> +        (dev->setup_state == SETUP_STATE_DATA &&

Fails to build, SETUP_STATE_DATA is not defined here.

I think we can simply drop that check, index should never ever be larger
than len, no matter what the state is.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Michael S. Tsirkin]

On Tue, May 13, 2014 at 09:50:09AM +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > +        (dev->setup_state == SETUP_STATE_DATA &&
> 
> Fails to build, SETUP_STATE_DATA is not defined here.
> 
> I think we can simply drop that check, index should never ever be larger
> than len, no matter what the state is.
> 
> cheers,
>   Gerd

I'm confused by usb_generic_async_ctrl_complete which can modify len
without touching index.
If index is stale it could get > len?
Could you hop on irc so we can discuss?

-- 
MST

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Gerd Hoffmann]

On Di, 2014-05-13 at 11:32 +0300, Michael S. Tsirkin wrote:
> On Tue, May 13, 2014 at 09:50:09AM +0200, Gerd Hoffmann wrote:
> >   Hi,
> > 
> > > +        (dev->setup_state == SETUP_STATE_DATA &&
> > 
> > Fails to build, SETUP_STATE_DATA is not defined here.
> > 
> > I think we can simply drop that check, index should never ever be larger
> > than len, no matter what the state is.
> > 
> > cheers,
> >   Gerd
> 
> I'm confused by usb_generic_async_ctrl_complete which can modify len
> without touching index.

only in setup state, before any data from/to the buffer is transfered,
so index is still zero at that point.

flow is this:

state_setup: len = $buflen, index = 0
state_data:  xfer %buf data, increase index up to len while doing so.
state_ack:   index == len
state_idle:  likewise.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Michael S. Tsirkin]

On Tue, May 13, 2014 at 10:44:45AM +0200, Gerd Hoffmann wrote:
> On Di, 2014-05-13 at 11:32 +0300, Michael S. Tsirkin wrote:
> > On Tue, May 13, 2014 at 09:50:09AM +0200, Gerd Hoffmann wrote:
> > >   Hi,
> > > 
> > > > +        (dev->setup_state == SETUP_STATE_DATA &&
> > > 
> > > Fails to build, SETUP_STATE_DATA is not defined here.
> > > 
> > > I think we can simply drop that check, index should never ever be larger
> > > than len, no matter what the state is.
> > > 
> > > cheers,
> > >   Gerd
> > 
> > I'm confused by usb_generic_async_ctrl_complete which can modify len
> > without touching index.
> 
> only in setup state, before any data from/to the buffer is transfered,
> so index is still zero at that point.


And SETUP_STATE_PARAM?

> flow is this:
> 
> state_setup: len = $buflen, index = 0
> state_data:  xfer %buf data, increase index up to len while doing so.
> state_ack:   index == len
> state_idle:  likewise.
> 
> cheers,
>   Gerd
> 

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Gerd Hoffmann]

  Hi,


> And SETUP_STATE_PARAM?

Shortcut for small control transfers on xhci.  Doesn't go through the
idle -> setup -> data -> ack state engine.

security-wise: you can't go from 'param' to 'data' without 'setup'
inbetween.  beside that index should still be zero at the point where
len is modified (simliar to the other place in setup state).

side note: changing len should not happen in normal operation, only with
a malicious / buggy guest.  It happens in case the guest claims the data
transfer is larger than the buffer supplied by the guest.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] usb: fix up post load checks

[Michael S. Tsirkin]

On Mon, May 12, 2014 at 03:16:07PM +0300, Michael S. Tsirkin wrote:
> Correct post load checks:
> 1. dev->setup_len == sizeof(dev->data_buf)
>     seems fine, no need to fail migration
> 2. When state is DATA, passing index > len
>    will cause memcpy with negative length,
>    resulting in heap overflow
> 
> First of the issues was reported by dgilbert.
> 
> Reported-by: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

This is CVE-2014-3461 

> ---
>  hw/usb/bus.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/usb/bus.c b/hw/usb/bus.c
> index e48b19f..2721719 100644
> --- a/hw/usb/bus.c
> +++ b/hw/usb/bus.c
> @@ -51,8 +51,9 @@ static int usb_device_post_load(void *opaque, int version_id)
>      }
>      if (dev->setup_index < 0 ||
>          dev->setup_len < 0 ||
> -        dev->setup_index >= sizeof(dev->data_buf) ||
> -        dev->setup_len >= sizeof(dev->data_buf)) {
> +        (dev->setup_state == SETUP_STATE_DATA &&
> +         dev->setup_index > dev->setup_len) ||
> +        dev->setup_len > sizeof(dev->data_buf)) {
>          return -EINVAL;
>      }
>      return 0;
> -- 
> MST