> -----Original Message----- > From: Jan Beulich [mailto:[email protected]] > Sent: 11 March 2019 10:45 > To: Paul Durrant <[email protected]> > Cc: Andrew Cooper <[email protected]>; Roger Pau Monne > <[email protected]>; Wei Liu > <[email protected]>; xen-devel <[email protected]> > Subject: RE: [PATCH] x86/HVM: don't crash guest in hvmemul_find_mmio_cache() > > >>> On 11.03.19 at 11:11, <[email protected]> wrote: > >> -----Original Message----- > >> From: Jan Beulich [mailto:[email protected]] > >> Sent: 11 March 2019 09:56 > >> To: xen-devel <[email protected]> > >> Cc: Andrew Cooper <[email protected]>; Paul Durrant > > <[email protected]>; Roger Pau Monne > >> <[email protected]>; Wei Liu <[email protected]> > >> 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 <[email protected]> > >> > >> --- 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 <[email protected]> > > Jan > _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
