| Message ID | 20220427184814.2204513-4-ricarkol@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic: Misc ITS fixes | expand |
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@google.com> > --- > 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
On Tue, May 03, 2022 at 07:40:46PM +0200, Eric Auger wrote: > 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. Thanks, will clarify this. I was just focusing on that positive ITS error code being treated as success by the caller. > > 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". Ah, definitely. This was incorrect then. > > > > 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@google.com> > > --- > > 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 > Thanks, Ricardo
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. + * 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; }
Restoring a corrupted collection entry is being ignored and treated as 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. 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. 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@google.com> --- arch/arm64/kvm/vgic/vgic-its.c | 35 ++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-)