Hi Marc,
On 16/07/16 11:08, Marc Zyngier wrote:
> On Fri, 15 Jul 2016 12:43:36 +0100
>> +/*
>> + * Check whether a device ID can be stored into the guest device tables.
>> + * For a direct table this is pretty easy, but gets a bit nasty for
>> + * indirect tables. We check whether the resulting guest physical address
>> + * is actually valid (covered by a memslot and guest accessbible).
>> + * For this we have to read the respective first level entry.
>> + */
>> +static bool vgic_its_check_device_id(struct kvm *kvm, struct vgic_its *its,
>> + int device_id)
>> +{
>> + u64 r = its->baser_device_table;
>> + int nr_entries = GITS_BASER_NR_PAGES(r) * SZ_64K;
>> + int index;
>> + u64 indirect_ptr;
>> + gfn_t gfn;
>> +
>> +
>> + if (!(r & GITS_BASER_INDIRECT))
>> + return device_id < (nr_entries / GITS_BASER_ENTRY_SIZE(r));
>> +
>> + /* calculate and check the index into the 1st level */
>> + index = device_id / (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
>> + if (index >= (nr_entries / sizeof(u64)))
>> + return false;
>> +
>> + /* Each 1st level entry is represented by a 64-bit value. */
>> + if (!kvm_read_guest(kvm,
>> + BASER_ADDRESS(r) + index *
>> sizeof(indirect_ptr),
>> + &indirect_ptr, sizeof(indirect_ptr)))
>> + return false;
>
> Careful. The ITS tables are written in LE format, so you need a
>
> indirect_ptr = le64_to_cpu(indirect_ptr);
>
> to cater for the LE-on-BE case.
Oh right, endianness. Thanks for pointing that out!
>> +
>> + /* check the valid bit of the first level entry */
>> + if (!(indirect_ptr & BIT_ULL(63)))
>> + return false;
>> +
>> + /*
>> + * Mask the guest physical address and calculate the frame number.
>> + * Any address beyond our supported 48 bits of PA will be caught
>> + * by the actual check in the final step.
>> + */
>> + gfn = (indirect_ptr & GENMASK_ULL(51, 16)) >> PAGE_SHIFT;
>
> Another gotcha: Here, you're only checking for the CPU page that covers
> the beginning of the ITS page. If the CPU page size is smaller, you may
> end-up being out of bounds. You need something like:
>
> l2_index = device_id % (SZ_64K / GITS_BASER_ENTRY_SIZE(r));
> gfn = ((indirect_ptr & GENMASK_ULL(51, 16)) +
> l2_index * GITS_BASER_ENTRY_SIZE(r)) >> PAGE_SHIFT;
>
> so that you check the presence of the actual location you're virtually
> touching.
>
Right, that nasty case came to me as well after sending the patches.
I had something like "gfn += ...." plus a comment in mind, but that's
purely cosmetical.
So do you want me to send out another version with those fixes (and
possibly the usage of vgic_get_irq_kref() above)?
Are there more comments?
Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
