[Qemu-devel] [PATCH] aio-win32: avoid out-of-bounds access to the events array

[Qemu-devel] [PATCH] aio-win32: avoid out-of-bounds access to the events array

[Paolo Bonzini]

If ret is WAIT_TIMEOUT and there was an event returned by select(),
we can write to a location after the end of the array.  But in
that case we can retry the WaitForMultipleObjects call with the
same set of events, so just move the event[ret - WAIT_OBJECT_0]
assignment inside the existin conditional.

Reported-by: TeLeMan <geleman@gmail.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 aio-win32.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/aio-win32.c b/aio-win32.c
index 61e3d2d..89f9e1f 100644
--- a/aio-win32.c
+++ b/aio-win32.c
@@ -335,6 +335,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
         event = NULL;
         if ((DWORD) (ret - WAIT_OBJECT_0) < count) {
             event = events[ret - WAIT_OBJECT_0];
+            events[ret - WAIT_OBJECT_0] = events[--count];
         } else if (!have_select_revents) {
             break;
         }
@@ -343,9 +344,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
         blocking = false;
 
         progress |= aio_dispatch_handlers(ctx, event);
-
-        /* Try again, but only call each handler once.  */
-        events[ret - WAIT_OBJECT_0] = events[--count];
     }
 
     progress |= timerlistgroup_run_timers(&ctx->tlg);
-- 
2.1.0

Re: [Qemu-devel] [PATCH] aio-win32: avoid out-of-bounds access to the events array

[TeLeMan]

On Mon, Sep 15, 2014 at 8:52 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> If ret is WAIT_TIMEOUT and there was an event returned by select(),
> we can write to a location after the end of the array.  But in
> that case we can retry the WaitForMultipleObjects call with the
> same set of events, so just move the event[ret - WAIT_OBJECT_0]
> assignment inside the existin conditional.
>
> Reported-by: TeLeMan <geleman@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio-win32.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/aio-win32.c b/aio-win32.c
> index 61e3d2d..89f9e1f 100644
> --- a/aio-win32.c
> +++ b/aio-win32.c
> @@ -335,6 +335,7 @@ bool aio_poll(AioContext *ctx, bool blocking)
>          event = NULL;
>          if ((DWORD) (ret - WAIT_OBJECT_0) < count) {
>              event = events[ret - WAIT_OBJECT_0];
> +            events[ret - WAIT_OBJECT_0] = events[--count];
>          } else if (!have_select_revents) {
>              break;
>          }
> @@ -343,9 +344,6 @@ bool aio_poll(AioContext *ctx, bool blocking)
>          blocking = false;
>
>          progress |= aio_dispatch_handlers(ctx, event);
> -
> -        /* Try again, but only call each handler once.  */
> -        events[ret - WAIT_OBJECT_0] = events[--count];
>      }
>
>      progress |= timerlistgroup_run_timers(&ctx->tlg);
> --
> 2.1.0
>

Reviewed-by: TeLeMan <geleman@gmail.com>

Re: [Qemu-devel] [PATCH] aio-win32: avoid out-of-bounds access to the events array

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 661 bytes --]

On Mon, Sep 15, 2014 at 02:52:58PM +0200, Paolo Bonzini wrote:
> If ret is WAIT_TIMEOUT and there was an event returned by select(),
> we can write to a location after the end of the array.  But in
> that case we can retry the WaitForMultipleObjects call with the
> same set of events, so just move the event[ret - WAIT_OBJECT_0]
> assignment inside the existin conditional.
> 
> Reported-by: TeLeMan <geleman@gmail.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  aio-win32.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]