Hi Ricardo,
On 4/27/22 20:48, Ricardo Koller wrote:
> Restoring a corrupted collection entry is being ignored and treated as
maybe precise what is a corrupted ITE (out of range id or not matching
guest RAM)
> success. More specifically, vgic_its_restore_cte failure is treated as
> success by vgic_its_restore_collection_table. vgic_its_restore_cte uses
> a positive number to return ITS error codes, and +1 to return success.
Not fully correct as vgic_its_restore_cte() also returns a bunch of
generic negative error codes. vgic_its_alloc_collection() only returns
one positive ITS error code.
> The caller then uses "ret > 0" to check for success. An additional issue
> is that invalid entries return 0 and although that doesn't fail the
> restore, it leads to skipping all the next entries.
Isn't what we want. If I remember correctly an invalid entry corresponds
to the end of the collection table, hence the break.
see vgic_its_save_collection_table() and "add a last dummy element with
valid bit unset".
>
> Fix this by having vgic_its_restore_cte return negative numbers on
> error, and 0 on success (which includes skipping an invalid entry).
> While doing that, also fix alloc_collection return codes to not mix ITS
> error codes (positive numbers) and generic error codes (negative
> numbers).
>
> Signed-off-by: Ricardo Koller <ricarkol@xxxxxxxxxx>
> ---
> arch/arm64/kvm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index fb2d26a73880..86c26aaa8275 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -999,15 +999,16 @@ static bool vgic_its_check_event_id(struct vgic_its *its, struct its_device *dev
> return __is_visible_gfn_locked(its, gpa);
> }
>
> +/*
> + * Adds a new collection into the ITS collection table.
nit: s/Adds/Add here and below
> + * Returns 0 on success, and a negative error value for generic errors.
> + */
> static int vgic_its_alloc_collection(struct vgic_its *its,
> struct its_collection **colp,
> u32 coll_id)
> {
> struct its_collection *collection;
>
> - if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> - return E_ITS_MAPC_COLLECTION_OOR;
> -
> collection = kzalloc(sizeof(*collection), GFP_KERNEL_ACCOUNT);
> if (!collection)
> return -ENOMEM;
> @@ -1101,7 +1102,12 @@ static int vgic_its_cmd_handle_mapi(struct kvm *kvm, struct vgic_its *its,
>
> collection = find_collection(its, coll_id);
> if (!collection) {
> - int ret = vgic_its_alloc_collection(its, &collection, coll_id);
> + int ret;
> +
> + if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> + return E_ITS_MAPC_COLLECTION_OOR;
> +
> + ret = vgic_its_alloc_collection(its, &collection, coll_id);
> if (ret)
> return ret;
> new_coll = collection;
> @@ -1256,6 +1262,10 @@ static int vgic_its_cmd_handle_mapc(struct kvm *kvm, struct vgic_its *its,
> if (!collection) {
> int ret;
>
> + if (!vgic_its_check_id(its, its->baser_coll_table,
> + coll_id, NULL))
> + return E_ITS_MAPC_COLLECTION_OOR;
> +
> ret = vgic_its_alloc_collection(its, &collection,
> coll_id);
> if (ret)
> @@ -2497,6 +2507,10 @@ static int vgic_its_save_cte(struct vgic_its *its,
> return kvm_write_guest_lock(its->dev->kvm, gpa, &val, esz);
> }
>
> +/*
> + * Restores a collection entry into the ITS collection table.
> + * Returns 0 on success, and a negative error value for generic errors.
> + */
> static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> {
> struct its_collection *collection;
> @@ -2511,7 +2525,7 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> return ret;
> val = le64_to_cpu(val);
> if (!(val & KVM_ITS_CTE_VALID_MASK))
> - return 0;
> + return 0; /* invalid entry, skip it */
>
> target_addr = (u32)(val >> KVM_ITS_CTE_RDBASE_SHIFT);
> coll_id = val & KVM_ITS_CTE_ICID_MASK;
> @@ -2523,11 +2537,15 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
> collection = find_collection(its, coll_id);
> if (collection)
> return -EEXIST;
> +
> + if (!vgic_its_check_id(its, its->baser_coll_table, coll_id, NULL))
> + return -EINVAL;
> +
> ret = vgic_its_alloc_collection(its, &collection, coll_id);
> if (ret)
> return ret;
> collection->target_addr = target_addr;
> - return 1;
> + return 0;
> }
>
> /**
> @@ -2593,15 +2611,12 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>
> while (read < max_size) {
> ret = vgic_its_restore_cte(its, gpa, cte_esz);
> - if (ret <= 0)
> + if (ret < 0)
> break;
> gpa += cte_esz;
> read += cte_esz;
> }
>
> - if (ret > 0)
> - return 0;
> -
> return ret;
> }
>
Thanks
Eric
