Re: [Xen-devel] [PATCH] x86/HVM: don't crash guest in hvmemul_find_mmio_cache()

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 11 March 2019 09:56
> To: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Paul Durrant 
> <Paul.Durrant@xxxxxxxxxx>; Roger Pau Monne
> <roger.pau@xxxxxxxxxx>; Wei Liu <wei.liu2@xxxxxxxxxx>
> Subject: [PATCH] x86/HVM: don't crash guest in hvmemul_find_mmio_cache()
> 
> Commit 35a61c05ea ("x86emul: adjust handling of AVX2 gathers") builds
> upon the fact that the domain will actually survive running out of MMIO
> result buffer space. Drop the domain_crash() invocation. Also delay
> incrementing of the usage counter, such that the function can't possibly
> use/return an out-of-bounds slot/pointer in case execution subsequently
> makes it into the function again without a prior reset of state.
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> 
> --- a/xen/arch/x86/hvm/emulate.c
> +++ b/xen/arch/x86/hvm/emulate.c
> @@ -966,12 +966,11 @@ static struct hvm_mmio_cache *hvmemul_fi
>              return cache;
>      }
> 
> -    i = vio->mmio_cache_count++;
> +    i = vio->mmio_cache_count;
>      if( i == ARRAY_SIZE(vio->mmio_cache) )
> -    {
> -        domain_crash(current->domain);
>          return NULL;
> -    }
> +
> +    ++vio->mmio_cache_count;

AFAICT this isn't going to stop the for loop at the top of the function 
accessing one entry beyond the bounds of the array. If you're going to remove 
the domain_crash() then I think you also need to move the bounds check to the 
top of the function.

  Paul

> 
>      cache = &vio->mmio_cache[i];
>      memset(cache, 0, sizeof (*cache));
> 


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel