| Message ID | 1399896829-16617-1-git-send-email-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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?
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
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 >
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
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
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;
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(-)