Re: [PATCH v2 1/4] KVM: arm64: vgic: Check that new ITEs could be saved in guest memory

[Eric Auger <eric.auger@redhat.com>]

Hi Ricardo,

On 4/27/22 20:48, Ricardo Koller wrote:
> Try to improve the predictability of ITS save/restores by failing
> commands that would lead to failed saves. More specifically, fail any
> command that adds an entry into an ITS table that is not in guest
> memory, which would otherwise lead to a failed ITS save ioctl. There
> are already checks for collection and device entries, but not for
> ITEs.  Add the corresponding check for the ITT when adding ITEs.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 51 ++++++++++++++++++++++++----------
>  1 file changed, 37 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 2e13402be3bd..e14790750958 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -894,6 +894,18 @@ static int vgic_its_cmd_handle_movi(struct kvm *kvm, struct vgic_its *its,
>  	return update_affinity(ite->irq, vcpu);
>  }
>  
> +static bool __is_visible_gfn_locked(struct vgic_its *its, gpa_t gpa)
> +{
> +	gfn_t gfn = gpa >> PAGE_SHIFT;
> +	int idx;
> +	bool ret;
> +
> +	idx = srcu_read_lock(&its->dev->kvm->srcu);
> +	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> +	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> +	return ret;
> +}
> +
>  /*
>   * Check whether an ID can be stored into the corresponding guest table.
>   * For a direct table this is pretty easy, but gets a bit nasty for
> @@ -908,9 +920,7 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	u64 indirect_ptr, type = GITS_BASER_TYPE(baser);
>  	phys_addr_t base = GITS_BASER_ADDR_48_to_52(baser);
>  	int esz = GITS_BASER_ENTRY_SIZE(baser);
> -	int index, idx;
> -	gfn_t gfn;
> -	bool ret;
> +	int index;
>  
>  	switch (type) {
>  	case GITS_BASER_TYPE_DEVICE:
> @@ -933,12 +943,11 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  			return false;
>  
>  		addr = base + id * esz;
> -		gfn = addr >> PAGE_SHIFT;
>  
>  		if (eaddr)
>  			*eaddr = addr;
>  
> -		goto out;
> +		return __is_visible_gfn_locked(its, addr);
>  	}
>  
>  	/* calculate and check the index into the 1st level */
> @@ -964,16 +973,30 @@ static bool vgic_its_check_id(struct vgic_its *its, u64 baser, u32 id,
>  	/* Find the address of the actual entry */
>  	index = id % (SZ_64K / esz);
>  	indirect_ptr += index * esz;
> -	gfn = indirect_ptr >> PAGE_SHIFT;
>  
>  	if (eaddr)
>  		*eaddr = indirect_ptr;
>  
> -out:
> -	idx = srcu_read_lock(&its->dev->kvm->srcu);
> -	ret = kvm_is_visible_gfn(its->dev->kvm, gfn);
> -	srcu_read_unlock(&its->dev->kvm->srcu, idx);
> -	return ret;
> +	return __is_visible_gfn_locked(its, indirect_ptr);
> +}
> +
> +/*
> + * Check whether an event ID can be stored in the corresponding Interrupt
> + * Translation Table, which starts at device->itt_addr.
> + */
> +static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *device,
> +		u32 event_id)
> +{
> +	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
> +	int ite_esz = abi->ite_esz;
> +	gpa_t gpa;
> +
> +	/* max table size is: BIT_ULL(device->num_eventid_bits) * ite_esz */
> +	if (event_id >= BIT_ULL(device->num_eventid_bits))
> +		return false;
> +
> +	gpa = device->itt_addr + event_id * ite_esz;
> +	return __is_visible_gfn_locked(its, gpa);
>  }
>  
>  static int vgic_its_alloc_collection(struct vgic_its *its,
> @@ -1061,9 +1084,6 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	if (!device)
>  		return E_ITS_MAPTI_UNMAPPED_DEVICE;
>  
> -	if (event_id >= BIT_ULL(device->num_eventid_bits))
> -		return E_ITS_MAPTI_ID_OOR;
I would put
    if (!vgic_its_check_event_id(its, device, event_id))
        return E_ITS_MAPTI_ID_OOR;
here instead of after since if the evend_id not correct, no use to look
the ite for instance.
> -
>  	if (its_cmd_get_command(its_cmd) == GITS_CMD_MAPTI)
>  		lpi_nr = its_cmd_get_physical_id(its_cmd);
>  	else
> @@ -1076,6 +1096,9 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>  	if (find_ite(its, device_id, event_id))
>  		return 0;
>  
> +	if (!vgic_its_check_event_id(its, device, event_id))
> +		return E_ITS_MAPTI_ID_OOR;
> +
>  	collection = find_collection(its, coll_id);
>  	if (!collection) {
>  		int ret = vgic_its_alloc_collection(its, &collection, coll_id);
Besides look good to me
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

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