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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> 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.

Jan



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