From: Marc Zyngier <maz@xxxxxxxxxx>
commit 3b2c81d5feb250dfdcb0ef5825319f36c29f8336 upstream.
The ITS ABI infrastructure allows for some pretty lax code, where
the size of the data doesn't have to match the size of the entry,
potentially leading to a collection of interesting bugs.
Commit 7fe28d7e68f9 ("KVM: arm64: vgic-its: Add a data length check
in vgic_its_save_*") added some checks, but starts by implicitly
casting all writes to a 64bit value, hiding some of the issues.
Instead, introduce macros that will check the data type actually used
for dealing with the table entries. The macros are taking a symbolic
entry type that is used to fetch the size of the entry type for the
current ABI. This immediately catches a couple of low-impact gotchas
(zero values that are implicitly 32bit), easy enough to fix.
Given that we currently only have a single ABI, hardcode a couple of
BUILD_BUG_ON()s that will fire if we use anything but a 64bit quantity,
and some (currently unreachable) fallback code that may become useful
one day.
Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
Link: https://lore.kernel.org/r/20241117165757.247686-5-maz@xxxxxxxxxx
Signed-off-by: Oliver Upton <oliver.upton@xxxxxxxxx>
Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
---
arch/arm64/kvm/vgic/vgic-its.c | 69 +++++++++++++++++++++++++++++------------
arch/arm64/kvm/vgic/vgic.h | 23 -------------
2 files changed, 50 insertions(+), 42 deletions(-)
--- a/arch/arm64/kvm/vgic/vgic-its.c
+++ b/arch/arm64/kvm/vgic/vgic-its.c
@@ -31,6 +31,41 @@ static int vgic_its_commit_v0(struct vgi
static int update_lpi_config(struct kvm *kvm, struct vgic_irq *irq,
struct kvm_vcpu *filter_vcpu, bool needs_inv);
+#define vgic_its_read_entry_lock(i, g, valp, t) \
+ ({ \
+ int __sz = vgic_its_get_abi(i)->t##_esz; \
+ struct kvm *__k = (i)->dev->kvm; \
+ int __ret; \
+ \
+ BUILD_BUG_ON(NR_ITS_ABIS == 1 && \
+ sizeof(*(valp)) != ABI_0_ESZ); \
+ if (NR_ITS_ABIS > 1 && \
+ KVM_BUG_ON(__sz != sizeof(*(valp)), __k)) \
+ __ret = -EINVAL; \
+ else \
+ __ret = kvm_read_guest_lock(__k, (g), \
+ valp, __sz); \
+ __ret; \
+ })
+
+#define vgic_its_write_entry_lock(i, g, val, t) \
+ ({ \
+ int __sz = vgic_its_get_abi(i)->t##_esz; \
+ struct kvm *__k = (i)->dev->kvm; \
+ typeof(val) __v = (val); \
+ int __ret; \
+ \
+ BUILD_BUG_ON(NR_ITS_ABIS == 1 && \
+ sizeof(__v) != ABI_0_ESZ); \
+ if (NR_ITS_ABIS > 1 && \
+ KVM_BUG_ON(__sz != sizeof(__v), __k)) \
+ __ret = -EINVAL; \
+ else \
+ __ret = vgic_write_guest_lock(__k, (g), \
+ &__v, __sz); \
+ __ret; \
+ })
+
/*
* Creates a new (reference to a) struct vgic_irq for a given LPI.
* If this LPI is already mapped on another ITS, we increase its refcount
@@ -794,7 +829,7 @@ static int vgic_its_cmd_handle_discard(s
its_free_ite(kvm, ite);
- return vgic_its_write_entry_lock(its, gpa, 0, ite_esz);
+ return vgic_its_write_entry_lock(its, gpa, 0ULL, ite);
}
return E_ITS_DISCARD_UNMAPPED_INTERRUPT;
@@ -1143,7 +1178,6 @@ static int vgic_its_cmd_handle_mapd(stru
bool valid = its_cmd_get_validbit(its_cmd);
u8 num_eventid_bits = its_cmd_get_size(its_cmd);
gpa_t itt_addr = its_cmd_get_ittaddr(its_cmd);
- int dte_esz = vgic_its_get_abi(its)->dte_esz;
struct its_device *device;
gpa_t gpa;
@@ -1168,7 +1202,7 @@ static int vgic_its_cmd_handle_mapd(stru
* is an error, so we are done in any case.
*/
if (!valid)
- return vgic_its_write_entry_lock(its, gpa, 0, dte_esz);
+ return vgic_its_write_entry_lock(its, gpa, 0ULL, dte);
device = vgic_its_alloc_device(its, device_id, itt_addr,
num_eventid_bits);
@@ -2090,7 +2124,7 @@ static int scan_its_table(struct vgic_it
* vgic_its_save_ite - Save an interrupt translation entry at @gpa
*/
static int vgic_its_save_ite(struct vgic_its *its, struct its_device *dev,
- struct its_ite *ite, gpa_t gpa, int ite_esz)
+ struct its_ite *ite, gpa_t gpa)
{
u32 next_offset;
u64 val;
@@ -2101,7 +2135,7 @@ static int vgic_its_save_ite(struct vgic
ite->collection->collection_id;
val = cpu_to_le64(val);
- return vgic_its_write_entry_lock(its, gpa, val, ite_esz);
+ return vgic_its_write_entry_lock(its, gpa, val, ite);
}
/**
@@ -2201,7 +2235,7 @@ static int vgic_its_save_itt(struct vgic
if (ite->irq->hw && !kvm_vgic_global_state.has_gicv4_1)
return -EACCES;
- ret = vgic_its_save_ite(its, device, ite, gpa, ite_esz);
+ ret = vgic_its_save_ite(its, device, ite, gpa);
if (ret)
return ret;
}
@@ -2240,10 +2274,9 @@ static int vgic_its_restore_itt(struct v
* @its: ITS handle
* @dev: ITS device
* @ptr: GPA
- * @dte_esz: device table entry size
*/
static int vgic_its_save_dte(struct vgic_its *its, struct its_device *dev,
- gpa_t ptr, int dte_esz)
+ gpa_t ptr)
{
u64 val, itt_addr_field;
u32 next_offset;
@@ -2256,7 +2289,7 @@ static int vgic_its_save_dte(struct vgic
(dev->num_eventid_bits - 1));
val = cpu_to_le64(val);
- return vgic_its_write_entry_lock(its, ptr, val, dte_esz);
+ return vgic_its_write_entry_lock(its, ptr, val, dte);
}
/**
@@ -2332,10 +2365,8 @@ static int vgic_its_device_cmp(void *pri
*/
static int vgic_its_save_device_tables(struct vgic_its *its)
{
- const struct vgic_its_abi *abi = vgic_its_get_abi(its);
u64 baser = its->baser_device_table;
struct its_device *dev;
- int dte_esz = abi->dte_esz;
if (!(baser & GITS_BASER_VALID))
return 0;
@@ -2354,7 +2385,7 @@ static int vgic_its_save_device_tables(s
if (ret)
return ret;
- ret = vgic_its_save_dte(its, dev, eaddr, dte_esz);
+ ret = vgic_its_save_dte(its, dev, eaddr);
if (ret)
return ret;
}
@@ -2435,7 +2466,7 @@ static int vgic_its_restore_device_table
static int vgic_its_save_cte(struct vgic_its *its,
struct its_collection *collection,
- gpa_t gpa, int esz)
+ gpa_t gpa)
{
u64 val;
@@ -2444,7 +2475,7 @@ static int vgic_its_save_cte(struct vgic
collection->collection_id);
val = cpu_to_le64(val);
- return vgic_its_write_entry_lock(its, gpa, val, esz);
+ return vgic_its_write_entry_lock(its, gpa, val, cte);
}
/*
@@ -2452,7 +2483,7 @@ static int vgic_its_save_cte(struct vgic
* Return +1 on success, 0 if the entry was invalid (which should be
* interpreted as end-of-table), and a negative error value for generic errors.
*/
-static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
+static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa)
{
struct its_collection *collection;
struct kvm *kvm = its->dev->kvm;
@@ -2460,7 +2491,7 @@ static int vgic_its_restore_cte(struct v
u64 val;
int ret;
- ret = vgic_its_read_entry_lock(its, gpa, &val, esz);
+ ret = vgic_its_read_entry_lock(its, gpa, &val, cte);
if (ret)
return ret;
val = le64_to_cpu(val);
@@ -2507,7 +2538,7 @@ static int vgic_its_save_collection_tabl
max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
list_for_each_entry(collection, &its->collection_list, coll_list) {
- ret = vgic_its_save_cte(its, collection, gpa, cte_esz);
+ ret = vgic_its_save_cte(its, collection, gpa);
if (ret)
return ret;
gpa += cte_esz;
@@ -2521,7 +2552,7 @@ static int vgic_its_save_collection_tabl
* table is not fully filled, add a last dummy element
* with valid bit unset
*/
- return vgic_its_write_entry_lock(its, gpa, 0, cte_esz);
+ return vgic_its_write_entry_lock(its, gpa, 0ULL, cte);
}
/*
@@ -2546,7 +2577,7 @@ static int vgic_its_restore_collection_t
max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
while (read < max_size) {
- ret = vgic_its_restore_cte(its, gpa, cte_esz);
+ ret = vgic_its_restore_cte(its, gpa);
if (ret <= 0)
break;
gpa += cte_esz;
--- a/arch/arm64/kvm/vgic/vgic.h
+++ b/arch/arm64/kvm/vgic/vgic.h
@@ -146,29 +146,6 @@ static inline int vgic_write_guest_lock(
return ret;
}
-static inline int vgic_its_read_entry_lock(struct vgic_its *its, gpa_t eaddr,
- u64 *eval, unsigned long esize)
-{
- struct kvm *kvm = its->dev->kvm;
-
- if (KVM_BUG_ON(esize != sizeof(*eval), kvm))
- return -EINVAL;
-
- return kvm_read_guest_lock(kvm, eaddr, eval, esize);
-
-}
-
-static inline int vgic_its_write_entry_lock(struct vgic_its *its, gpa_t eaddr,
- u64 eval, unsigned long esize)
-{
- struct kvm *kvm = its->dev->kvm;
-
- if (KVM_BUG_ON(esize != sizeof(eval), kvm))
- return -EINVAL;
-
- return vgic_write_guest_lock(kvm, eaddr, &eval, esize);
-}
-
/*
* This struct provides an intermediate representation of the fields contained
* in the GICH_VMCR and ICH_VMCR registers, such that code exporting the GIC
Patches currently in stable-queue which might be from maz@xxxxxxxxxx are
queue-6.12/kvm-arm64-vgic-v3-sanitise-guest-writes-to-gicr_invlpir.patch
queue-6.12/kvm-arm64-vgic-its-add-stronger-type-checking-to-the-its-entry-sizes.patch
queue-6.12/kvm-arm64-ignore-pmcntenset_el0-while-checking-for-overflow-status.patch
queue-6.12/kvm-arm64-get-rid-of-userspace_irqchip_in_use.patch
queue-6.12/kvm-arm64-don-t-retire-aborted-mmio-instruction.patch
queue-6.12/arm64-expose-id_aa64isar1_el1.xs-to-sanitised-featur.patch
