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@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>
Eric
