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 10:45
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; Roger Pau Monne 
> <roger.pau@xxxxxxxxxx>; Wei Liu
> <wei.liu2@xxxxxxxxxx>; xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Subject: RE: [PATCH] x86/HVM: don't crash guest in hvmemul_find_mmio_cache()
> 
> >>> On 11.03.19 at 11:11, <Paul.Durrant@xxxxxxxxxx> wrote:
> >>  -----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.
> 
> I don't follow:
> 
> static struct hvm_mmio_cache *hvmemul_find_mmio_cache(
>     struct hvm_vcpu_io *vio, unsigned long gla, uint8_t dir)
> {
>     unsigned int i;
>     struct hvm_mmio_cache *cache;
> 
>     for ( i = 0; i < vio->mmio_cache_count; i ++ )
> 
> This iterates up to (but not including) the recorded count of
> populated cache entries.

Sorry, yes I was misreading. The patch is safe as it stands.

Reviewed-by: Paul Durrant <paul.durrant@xxxxxxxxxx>

> 
> Jan
> 


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