Re: [PATCH] amd-iommu: Fix Guest CR3 Table following c/s 3a7947b6901

[Jan Beulich <jbeulich@xxxxxxxx>]

On 20.11.2020 15:37, Andrew Cooper wrote:
> On 20/11/2020 14:32, Jan Beulich wrote:
>> On 20.11.2020 15:19, Andrew Cooper wrote:
>>> --- a/xen/drivers/passthrough/amd/iommu_guest.c
>>> +++ b/xen/drivers/passthrough/amd/iommu_guest.c
>>> @@ -68,11 +68,39 @@ static void guest_iommu_disable(struct guest_iommu 
>>> *iommu)
>>>      iommu->enabled = 0;
>>>  }
>>>  
>>> -static uint64_t get_guest_cr3_from_dte(struct amd_iommu_dte *dte)
>>> +/*
>>> + * The Guest CR3 Table is a table written by the guest kernel, pointing at
>>> + * gCR3 values for PASID transactions to use.  The Device Table Entry 
>>> points
>>> + * at a system physical address.
>>> + *
>>> + * However, these helpers deliberately use untyped parameters without
>>> + * reference to gfn/mfn because they are used both for programming the real
>>> + * IOMMU, and interpreting a guests programming of its vIOMMU.
>>> + */
>>> +static uint64_t dte_get_gcr3_table(const struct amd_iommu_dte *dte)
>>>  {
>>>      return (((uint64_t)dte->gcr3_trp_51_31 << 31) |
>>>              (dte->gcr3_trp_30_15 << 15) |
>>> -            (dte->gcr3_trp_14_12 << 12)) >> PAGE_SHIFT;
>>> +            (dte->gcr3_trp_14_12 << 12));
>>> +}
>>> +
>>> +static void dte_set_gcr3_table(struct amd_iommu_dte *dte, uint16_t dom_id,
>>> +                               uint64_t addr, bool gv, uint8_t glx)
>>> +{
>>> +#define GCR3_MASK(hi, lo) (((1ul << ((hi) + 1)) - 1) & ~((1ul << (lo)) - 
>>> 1))
>>> +
>>> +    /* I bit must be set when gcr3 is enabled */
>>> +    dte->i = true;
>>> +
>>> +    dte->gcr3_trp_14_12 = MASK_EXTR(addr, GCR3_MASK(14, 12));
>>> +    dte->gcr3_trp_30_15 = MASK_EXTR(addr, GCR3_MASK(30, 15));
>>> +    dte->gcr3_trp_51_31 = MASK_EXTR(addr, GCR3_MASK(51, 31));
>>> +
>>> +    dte->domain_id = dom_id;
>>> +    dte->glx = glx;
>>> +    dte->gv = gv;
>>> +
>>> +#undef GCR3_MASK
>>>  }
>> I realize the question is somewhat unrelated, but aren't we updating
>> a live DTE here? If so, are there no ordering requirements between the
>> writes? Might be worth putting in barrier(s) right on this occasion.
> 
> I don't know.  Honestly, its not relevant either as this is code motion.

Well, okay:
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

> This entire file is full of security holes.  None of it is fit for use
> in its current form.

We're all aware of this, I think.

Jan