Re: [PATCH v2 2/4] KVM: arm64: vgic: Add more checks when restoring ITS tables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm



[Index of Archives]     [Linux KVM]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux