[PATCH v2 0/4] KVM: arm64: vgic: Misc ITS fixes

[PATCH v2 0/4] KVM: arm64: vgic: Misc ITS fixes

[Ricardo Koller]

The purpose of this series is to help debugging failed ITS saves and
restores.  Failures can be due to misconfiguration on the guest side:
tables with bogus base addresses, or the guest overwriting L1 indirect
tables. KVM can't do much in these cases, but one thing it can do to help
is raising errors as soon as possible. Here are a couple of cases where
KVM could do more:

- A command that adds an entry into an ITS table that is not in guest
  memory should fail, as any command should be treated as if it was
  actually saving entries in guest memory (KVM doesn't until saving).  KVM
  does this check for collections and devices (using vgic_its_check_id),
  but it doesn't for the ITT (Interrupt Translation Table). Commit #1 adds
  the missing check.

- Restoring the ITS tables does some checks for corrupted tables, but not
  as many as in a save.  For example, a device ID overflowing the table
  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, this would most
  likely result in some corrupted table; and this is what we would see from
  the host point of view:

	guest sets bogus baser addresses
	save ioctl
	restore ioctl
	save ioctl (fails)

  This failed save could happen many days after the first operation, so it
  would be hard to track down. Commit #2 adds some checks into restore:
  like checking that the ITE entries are not repeated.

- Restoring a corrupted collection entry is being ignored. Commit #3 fixes
  this while trying to organize the code so to make the bug more obvious
  next time.

Finally, failed restores should clean up all intermediate state. Commit #4
takes care of cleaning up everything created until the restore was deemed a
failure.

v1: https://lore.kernel.org/kvmarm/20220425185534.57011-1-ricarkol@google.com/
v1 -> v2:
- moved alloc_collection comment to its respective commit. [marc]
- refactored check_ite to reuse some code from check_id. [marc]
- rewrote all commit messages. [marc]

Tested with kvm-unit-tests ITS tests.

Ricardo Koller (4):
  KVM: arm64: vgic: Check that new ITEs could be saved in guest memory
  KVM: arm64: vgic: Add more checks when restoring ITS tables
  KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures
  KVM: arm64: vgic: Undo work in failed ITS restores

 arch/arm64/kvm/vgic/vgic-its.c | 112 +++++++++++++++++++++++++--------
 1 file changed, 87 insertions(+), 25 deletions(-)

-- 
2.36.0.464.gb9c8b46e94-goog

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

[PATCH v2 0/4] KVM: arm64: vgic: Misc ITS fixes

[Ricardo Koller]

The purpose of this series is to help debugging failed ITS saves and
restores.  Failures can be due to misconfiguration on the guest side:
tables with bogus base addresses, or the guest overwriting L1 indirect
tables. KVM can't do much in these cases, but one thing it can do to help
is raising errors as soon as possible. Here are a couple of cases where
KVM could do more:

- A command that adds an entry into an ITS table that is not in guest
  memory should fail, as any command should be treated as if it was
  actually saving entries in guest memory (KVM doesn't until saving).  KVM
  does this check for collections and devices (using vgic_its_check_id),
  but it doesn't for the ITT (Interrupt Translation Table). Commit #1 adds
  the missing check.

- Restoring the ITS tables does some checks for corrupted tables, but not
  as many as in a save.  For example, a device ID overflowing the table
  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, this would most
  likely result in some corrupted table; and this is what we would see from
  the host point of view:

	guest sets bogus baser addresses
	save ioctl
	restore ioctl
	save ioctl (fails)

  This failed save could happen many days after the first operation, so it
  would be hard to track down. Commit #2 adds some checks into restore:
  like checking that the ITE entries are not repeated.

- Restoring a corrupted collection entry is being ignored. Commit #3 fixes
  this while trying to organize the code so to make the bug more obvious
  next time.

Finally, failed restores should clean up all intermediate state. Commit #4
takes care of cleaning up everything created until the restore was deemed a
failure.

v1: https://lore.kernel.org/kvmarm/20220425185534.57011-1-ricarkol@google.com/
v1 -> v2:
- moved alloc_collection comment to its respective commit. [marc]
- refactored check_ite to reuse some code from check_id. [marc]
- rewrote all commit messages. [marc]

Tested with kvm-unit-tests ITS tests.

Ricardo Koller (4):
  KVM: arm64: vgic: Check that new ITEs could be saved in guest memory
  KVM: arm64: vgic: Add more checks when restoring ITS tables
  KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures
  KVM: arm64: vgic: Undo work in failed ITS restores

 arch/arm64/kvm/vgic/vgic-its.c | 112 +++++++++++++++++++++++++--------
 1 file changed, 87 insertions(+), 25 deletions(-)

-- 
2.36.0.464.gb9c8b46e94-goog

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

[Ricardo Koller]

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;
-
 	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);
-- 
2.36.0.464.gb9c8b46e94-goog

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

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

[Ricardo Koller]

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;
-
 	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);
-- 
2.36.0.464.gb9c8b46e94-goog

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

[Ricardo Koller]

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@google.com>
---
 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;
+
+	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;
+
+	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);
-- 
2.36.0.464.gb9c8b46e94-goog

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

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

[Ricardo Koller]

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@google.com>
---
 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;
+
+	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;
+
+	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);
-- 
2.36.0.464.gb9c8b46e94-goog

[PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

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(-)

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;
 }
 
-- 
2.36.0.464.gb9c8b46e94-goog

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

[PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

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(-)

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;
 }
 
-- 
2.36.0.464.gb9c8b46e94-goog

[PATCH v2 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Ricardo Koller]

Failed ITS restores should clean up all state restored until the
failure. There is some cleanup already present when failing to restore
some tables, but it's not complete. Add the missing cleanup.

Note that this changes the behavior in case of a failed restore of the
device tables.

	restore ioctl:
	1. restore collection tables
	2. restore device tables

With this commit, failures in 2. clean up everything created so far,
including state created by 1.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 86c26aaa8275..c35534d7393a 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2222,8 +2222,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
 
 	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
-	if (IS_ERR(irq))
+	if (IS_ERR(irq)) {
+		its_free_ite(kvm, ite);
 		return PTR_ERR(irq);
+	}
 	ite->irq = irq;
 
 	return offset;
@@ -2491,6 +2493,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (ret > 0)
 		ret = 0;
 
+	if (ret < 0)
+		vgic_its_free_device_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2617,6 +2622,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		read += cte_esz;
 	}
 
+	if (ret < 0)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2648,7 +2656,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	if (ret)
 		return ret;
 
-	return vgic_its_restore_device_tables(its);
+	ret = vgic_its_restore_device_tables(its);
+	if (ret)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+	return ret;
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
-- 
2.36.0.464.gb9c8b46e94-goog

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

[PATCH v2 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Ricardo Koller]

Failed ITS restores should clean up all state restored until the
failure. There is some cleanup already present when failing to restore
some tables, but it's not complete. Add the missing cleanup.

Note that this changes the behavior in case of a failed restore of the
device tables.

	restore ioctl:
	1. restore collection tables
	2. restore device tables

With this commit, failures in 2. clean up everything created so far,
including state created by 1.

Signed-off-by: Ricardo Koller <ricarkol@google.com>
---
 arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
index 86c26aaa8275..c35534d7393a 100644
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -2222,8 +2222,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
 		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
 
 	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
-	if (IS_ERR(irq))
+	if (IS_ERR(irq)) {
+		its_free_ite(kvm, ite);
 		return PTR_ERR(irq);
+	}
 	ite->irq = irq;
 
 	return offset;
@@ -2491,6 +2493,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 	if (ret > 0)
 		ret = 0;
 
+	if (ret < 0)
+		vgic_its_free_device_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2617,6 +2622,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		read += cte_esz;
 	}
 
+	if (ret < 0)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+
 	return ret;
 }
 
@@ -2648,7 +2656,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
 	if (ret)
 		return ret;
 
-	return vgic_its_restore_device_tables(its);
+	ret = vgic_its_restore_device_tables(its);
+	if (ret)
+		vgic_its_free_collection_list(its->dev->kvm, its);
+	return ret;
 }
 
 static int vgic_its_commit_v0(struct vgic_its *its)
-- 
2.36.0.464.gb9c8b46e94-goog

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

[Eric Auger]

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@google.com>
> ---
>  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

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

[Eric Auger]

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@google.com>
> ---
>  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@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

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

[Eric Auger]

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

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

[Eric Auger]

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

Re: [PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Eric Auger]

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

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

Re: [PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Eric Auger]

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

Re: [PATCH v2 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Eric Auger]

Hi Ricardo,

On 4/27/22 20:48, Ricardo Koller wrote:
> Failed ITS restores should clean up all state restored until the
> failure. There is some cleanup already present when failing to restore
> some tables, but it's not complete. Add the missing cleanup.
>
> Note that this changes the behavior in case of a failed restore of the
> device tables.
I assume this is acceptable
>
> 	restore ioctl:
> 	1. restore collection tables
> 	2. restore device tables
>
> With this commit, failures in 2. clean up everything created so far,
> including state created by 1.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 86c26aaa8275..c35534d7393a 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2222,8 +2222,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
>  
>  	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
> -	if (IS_ERR(irq))
> +	if (IS_ERR(irq)) {
> +		its_free_ite(kvm, ite);
>  		return PTR_ERR(irq);
> +	}
>  	ite->irq = irq;
>  
>  	return offset;
> @@ -2491,6 +2493,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	if (ret > 0)
>  		ret = 0;
>  
> +	if (ret < 0)
> +		vgic_its_free_device_list(its->dev->kvm, its);
> +
>  	return ret;
>  }
>  
> @@ -2617,6 +2622,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>  		read += cte_esz;
>  	}
>  
> +	if (ret < 0)
> +		vgic_its_free_collection_list(its->dev->kvm, its);
> +
>  	return ret;
>  }
>  
> @@ -2648,7 +2656,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  	if (ret)
>  		return ret;
>  
> -	return vgic_its_restore_device_tables(its);
> +	ret = vgic_its_restore_device_tables(its);
> +	if (ret)
> +		vgic_its_free_collection_list(its->dev->kvm, its);
> +	return ret;
>  }
>  
>  static int vgic_its_commit_v0(struct vgic_its *its)

Looks good to me.
Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric

Re: [PATCH v2 4/4] KVM: arm64: vgic: Undo work in failed ITS restores

[Eric Auger]

Hi Ricardo,

On 4/27/22 20:48, Ricardo Koller wrote:
> Failed ITS restores should clean up all state restored until the
> failure. There is some cleanup already present when failing to restore
> some tables, but it's not complete. Add the missing cleanup.
>
> Note that this changes the behavior in case of a failed restore of the
> device tables.
I assume this is acceptable
>
> 	restore ioctl:
> 	1. restore collection tables
> 	2. restore device tables
>
> With this commit, failures in 2. clean up everything created so far,
> including state created by 1.
>
> Signed-off-by: Ricardo Koller <ricarkol@google.com>
> ---
>  arch/arm64/kvm/vgic/vgic-its.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c
> index 86c26aaa8275..c35534d7393a 100644
> --- a/arch/arm64/kvm/vgic/vgic-its.c
> +++ b/arch/arm64/kvm/vgic/vgic-its.c
> @@ -2222,8 +2222,10 @@ static int vgic_its_restore_ite(struct vgic_its *its, u32 event_id,
>  		vcpu = kvm_get_vcpu(kvm, collection->target_addr);
>  
>  	irq = vgic_add_lpi(kvm, lpi_id, vcpu);
> -	if (IS_ERR(irq))
> +	if (IS_ERR(irq)) {
> +		its_free_ite(kvm, ite);
>  		return PTR_ERR(irq);
> +	}
>  	ite->irq = irq;
>  
>  	return offset;
> @@ -2491,6 +2493,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
>  	if (ret > 0)
>  		ret = 0;
>  
> +	if (ret < 0)
> +		vgic_its_free_device_list(its->dev->kvm, its);
> +
>  	return ret;
>  }
>  
> @@ -2617,6 +2622,9 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
>  		read += cte_esz;
>  	}
>  
> +	if (ret < 0)
> +		vgic_its_free_collection_list(its->dev->kvm, its);
> +
>  	return ret;
>  }
>  
> @@ -2648,7 +2656,10 @@ static int vgic_its_restore_tables_v0(struct vgic_its *its)
>  	if (ret)
>  		return ret;
>  
> -	return vgic_its_restore_device_tables(its);
> +	ret = vgic_its_restore_device_tables(its);
> +	if (ret)
> +		vgic_its_free_collection_list(its->dev->kvm, its);
> +	return ret;
>  }
>  
>  static int vgic_its_commit_v0(struct vgic_its *its)

Looks 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

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

[Ricardo Koller]

On Tue, May 03, 2022 at 07:14:24PM +0200, Eric Auger wrote:
> 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.

Thanks Eric. Will fix in v2.

> > -
> >  	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
> 

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

[Ricardo Koller]

On Tue, May 03, 2022 at 07:14:24PM +0200, Eric Auger wrote:
> 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.

Thanks Eric. Will fix in v2.

> > -
> >  	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

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

[Ricardo Koller]

On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote:
> 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@google.com>
> > ---
> >  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)

In that case, maybe we could ignore the new repeated entry? or
overwrite the old one?  find_ite() only returns the first (device_id,
event_id) match. So, it's like the new one is ignored already.  The
arm arm says this about MAPI commands in this situation:

    If there is an existing mapping for the EventID-DeviceID
    combination, behavior is UNPREDICTABLE.

And, just in case, the main reason for adding this check was to avoid
failing the next ITS save. The idea is to try to fail as soon as
possible, not in possibly many days during the next migration attempt.

> > +
> > +	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
>

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

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

[Ricardo Koller]

On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote:
> 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@google.com>
> > ---
> >  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)

In that case, maybe we could ignore the new repeated entry? or
overwrite the old one?  find_ite() only returns the first (device_id,
event_id) match. So, it's like the new one is ignored already.  The
arm arm says this about MAPI commands in this situation:

    If there is an existing mapping for the EventID-DeviceID
    combination, behavior is UNPREDICTABLE.

And, just in case, the main reason for adding this check was to avoid
failing the next ITS save. The idea is to try to fail as soon as
possible, not in possibly many days during the next migration attempt.

> > +
> > +	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
>

Thanks,
Ricardo

Re: [PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v2 3/4] KVM: arm64: vgic: Do not ignore vgic_its_restore_cte failures

[Ricardo Koller]

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

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

[Eric Auger]

Hi Ricardo,

On 5/4/22 19:01, Ricardo Koller wrote:
> On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote:
>> 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@google.com>
>>> ---
>>>  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)
> In that case, maybe we could ignore the new repeated entry? or
Maybe you can fail only in the case the ITE to be restored is different
from the existing one? otherwise ignore.

Eric
> overwrite the old one?  find_ite() only returns the first (device_id,
> event_id) match. So, it's like the new one is ignored already.  The
> arm arm says this about MAPI commands in this situation:
>
>     If there is an existing mapping for the EventID-DeviceID
>     combination, behavior is UNPREDICTABLE.
>
> And, just in case, the main reason for adding this check was to avoid
> failing the next ITS save. The idea is to try to fail as soon as
> possible, not in possibly many days during the next migration attempt.
>
>>> +
>>> +	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
>>
> Thanks,
> Ricardo
>

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

[Eric Auger]

Hi Ricardo,

On 5/4/22 19:01, Ricardo Koller wrote:
> On Tue, May 03, 2022 at 07:14:19PM +0200, Eric Auger wrote:
>> 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@google.com>
>>> ---
>>>  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)
> In that case, maybe we could ignore the new repeated entry? or
Maybe you can fail only in the case the ITE to be restored is different
from the existing one? otherwise ignore.

Eric
> overwrite the old one?  find_ite() only returns the first (device_id,
> event_id) match. So, it's like the new one is ignored already.  The
> arm arm says this about MAPI commands in this situation:
>
>     If there is an existing mapping for the EventID-DeviceID
>     combination, behavior is UNPREDICTABLE.
>
> And, just in case, the main reason for adding this check was to avoid
> failing the next ITS save. The idea is to try to fail as soon as
> possible, not in possibly many days during the next migration attempt.
>
>>> +
>>> +	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
>>
> Thanks,
> Ricardo
>

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