Re: [PATCH v6 02/16] x86/hvm: remove multiple open coded 'chunking' loops

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 08/07/2015 16:57, Paul Durrant wrote:
>> -----Original Message-----
>> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> bounces@lists.xen.org] On Behalf Of Jan Beulich
>> Sent: 08 July 2015 16:53
>> To: Andrew Cooper; Paul Durrant
>> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org)
>> Subject: Re: [Xen-devel] [PATCH v6 02/16] x86/hvm: remove multiple open
>> coded 'chunking' loops
>>
>>>>> On 03.07.15 at 18:25, <paul.durrant@citrix.com> wrote:
>>> +static int hvmemul_linear_mmio_access(
>>> +    unsigned long gla, unsigned int size, uint8_t dir, uint8_t *buffer,
>>> +    uint32_t pfec, struct hvm_emulate_ctxt *hvmemul_ctxt, bool_t
>> known_gpfn)
>>> +{
>>> +    struct hvm_vcpu_io *vio = &current->arch.hvm_vcpu.hvm_io;
>>> +    unsigned long offset = gla & ~PAGE_MASK;
>>> +    unsigned int chunk;
>>> +    paddr_t gpa;
>>> +    unsigned long one_rep = 1;
>>> +    int rc;
>>> +
>>> +    chunk = min_t(unsigned int, size, PAGE_SIZE - offset);
>>> +
>>> +    if ( known_gpfn )
>>> +        gpa = pfn_to_paddr(vio->mmio_gpfn) | offset;
>>> +    else
>>> +    {
>>> +        rc = hvmemul_linear_to_phys(gla, &gpa, chunk, &one_rep, pfec,
>>> +                                    hvmemul_ctxt);
>>> +        if ( rc != X86EMUL_OKAY )
>>> +            return rc;
>>> +    }
>>> +
>>> +    for ( ;; )
>>> +    {
>>> +        rc = hvmemul_phys_mmio_access(gpa, chunk, dir, buffer);
>>> +        if ( rc != X86EMUL_OKAY )
>>> +            break;
>>> +
>>> +        gla += chunk;
>>> +        buffer += chunk;
>>> +        size -= chunk;
>>> +
>>> +        if ( size == 0 )
>>> +            break;
>>> +
>>> +        ASSERT((gla & ~PAGE_MASK) == 0);
>> Does this really matter for the code below?
>>
>>> +        chunk = min_t(unsigned int, size, PAGE_SIZE);
>> Iirc Andrew had asked for this, but I still don't see why: "size" is the
>> width of an instruction operand, and hence won't even come close
>> to PAGE_SIZE.

The original version of the code asserted that size was less than
PAGE_SIZE around here.  This is not true in the general case, given a
for loop like this and can in principle be hit if we ever got into a
position of emulating an xsave instruction to an MMIO region.

This specific example is not as far fetched as it seems.  The VM
instrospection people are looking to emulate more and more instructions,
while the GVT-g are working on the mmio_write_dm side of things which
causes real RAM to be treated as MMIO from Xens point of view.

If we had some blanket sanity checks for size at the top of the
emulation calltree it would be less of an issue, but we don't and I have
a nagging feeing that assumptions like this are going to bite us in an
XSA-kind-of-way.

~Andrew