Hi Ricardo,
On 4/27/22 20:48, Ricardo Koller wrote:
> Try to improve the predictability of ITS save/restores (and debuggability
> of failed ITS saves) by failing early on restore when trying to read
> corrupted tables.
>
> Restoring the ITS tables does some checks for corrupted tables, but not as
> many as in a save: an overflowing device ID will be detected on save but
> not on restore. The consequence is that restoring a corrupted table won't
> be detected until the next save; including the ITS not working as expected
> after the restore. As an example, if the guest sets tables overlapping
> each other, which would most likely result in some corrupted table, this is
> what we would see from the host point of view:
>
> guest sets base addresses that overlap each other
> save ioctl
> restore ioctl
> save ioctl (fails)
>
> Ideally, we would like the first save to fail, but overlapping tables could
> actually be intended by the guest. So, let's at least fail on the restore
> with some checks: like checking that device and event IDs don't overflow
> their tables.
>
> Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
> ---
> arch/arm64/kvm/vgic/vgic-its.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index e14790750958..fb2d26a73880 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2198,6 +2198,12 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
> if (!collection)
> return -EINVAL;
>
> + if (find_ite(its, dev->device_id, event_id))
> + return -EINVAL;
Unsure about that. Nothing in the arm-vgic-its.rst doc says that the
KVM_DEV_ARM_ITS_RESTORE_TABLES ioctl cannot be called several times
(although obviously useless)
> +
> + if (!vgic_its_check_event_id(its, dev, event_id))
> + return -EINVAL;
> +
> ite = vgic_its_alloc_ite(dev, collection, event_id);
> if (IS_ERR(ite))
> return PTR_ERR(ite);
> @@ -2319,6 +2325,7 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> void *ptr, void *opaque)
> {
> struct its_device *dev;
> + u64 baser = its->baser_device_table;
> gpa_t itt_addr;
> u8 num_eventid_bits;
> u64 entry = *(u64 *)ptr;
> @@ -2339,6 +2346,12 @@ static int vgic_its_restore_dte(struct vgic_its *its, u32 id,
> /* dte entry is valid */
> offset = (entry & KVM_ITS_DTE_NEXT_MASK) >> KVM_ITS_DTE_NEXT_SHIFT;
>
> + if (find_its_device(its, id))
> + return -EINVAL;
same here.
> +
> + if (!vgic_its_check_id(its, baser, id, NULL))
> + return -EINVAL;
> +
> dev = vgic_its_alloc_device(its, id, itt_addr, num_eventid_bits);
> if (IS_ERR(dev))
> return PTR_ERR(dev);
Thanks
Eric
