Commit 3b2c81d5 authored by Marc Zyngier's avatar Marc Zyngier Committed by Oliver Upton
Browse files

KVM: arm64: vgic-its: Add stronger type-checking to the ITS entry sizes 



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 7fe28d7e ("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: default avatarMarc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20241117165757.247686-5-maz@kernel.org


Signed-off-by: default avatarOliver Upton <oliver.upton@linux.dev>
parent e7619f2a

arch/arm64/kvm/vgic/vgic-its.c

+50 −19
Original line number Diff line number Diff line
@@ -31,6 +31,41 @@ static int vgic_its_commit_v0(struct vgic_its *its); 
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(struct kvm *kvm, struct vgic_its *its, 


		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(struct kvm *kvm, struct vgic_its *its, 
	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(struct kvm *kvm, struct vgic_its *its, 
	 * 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_its *its, gpa_t base, int size, u32 esz, 
 * 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_its *its, struct its_device *dev, 
		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_its *its, struct its_device *device) 
		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 vgic_its *its, struct its_device *dev) 
 * @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_its *its, struct its_device *dev, 
		(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 *priv, const struct list_head *a, 
 */

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(struct vgic_its *its) 
		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_tables(struct vgic_its *its) 


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_its *its, 
	       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_its *its, 
 * 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 vgic_its *its, gpa_t gpa, int esz) 
	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_table(struct vgic_its *its) 
	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_table(struct vgic_its *its) 
	 * 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_table(struct vgic_its *its) 
	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;

arch/arm64/kvm/vgic/vgic.h

+0 −23
Original line number Diff line number Diff line
@@ -146,29 +146,6 @@ static inline int vgic_write_guest_lock(struct kvm *kvm, gpa_t gpa, 
	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