Commit ae98a4a9 authored by Marc Zyngier's avatar Marc Zyngier
Browse files

Merge branch kvm-arm64/sysreg-cleanup-5.20 into kvmarm-master/next

* kvm-arm64/sysreg-cleanup-5.20:
  : .
  : Long overdue cleanup of the sysreg userspace access,
  : with extra scrubbing on the vgic side of things.
  : From the cover letter:
  :
  : "Schspa Shi recently reported[1] that some of the vgic code interacting
  : with userspace was reading uninitialised stack memory, and although
  : that read wasn't used any further, it prompted me to revisit this part
  : of the code.
  :
  : Needless to say, this area of the kernel is pretty crufty, and shows a
  : bunch of issues in other parts of the KVM/arm64 infrastructure. This
  : series tries to remedy a bunch of them:
  :
  : - Sanitise the way we deal with sysregs from userspace: at the moment,
  :   each and every .set_user/.get_user callback has to implement its own
  :   userspace accesses (directly or indirectly). It'd be much better if
  :   that was centralised so that we can reason about it.
  :
  : - Enforce that all AArch64 sysregs are 64bit. Always. This was sort of
  :   implied by the code, but it took some effort to convince myself that
  :   this was actually the case.
  :
  : - Move the vgic-v3 sysreg userspace accessors to the userspace
  :   callbacks instead of hijacking the vcpu trap callback. This allows
  :   us to reuse the sysreg infrastructure.
  :
  : - Consolidate userspace accesses for both GICv2, GICv3 and common code
  :   as much as possible.
  :
  : - Cleanup a bunch of not-very-useful helpers, tidy up some of the code
  :   as we touch it.
  :
  : [1] https://lore.kernel.org/r/m2h740zz1i.fsf@gmail.com

"
  : .
  KVM: arm64: Get rid or outdated comments
  KVM: arm64: Descope kvm_arm_sys_reg_{get,set}_reg()
  KVM: arm64: Get rid of find_reg_by_id()
  KVM: arm64: vgic: Tidy-up calls to vgic_{get,set}_common_attr()
  KVM: arm64: vgic: Consolidate userspace access for base address setting
  KVM: arm64: vgic-v2: Add helper for legacy dist/cpuif base address setting
  KVM: arm64: vgic: Use {get,put}_user() instead of copy_{from.to}_user
  KVM: arm64: vgic-v2: Consolidate userspace access for MMIO registers
  KVM: arm64: vgic-v3: Consolidate userspace access for MMIO registers
  KVM: arm64: vgic-v3: Use u32 to manage the line level from userspace
  KVM: arm64: vgic-v3: Convert userspace accessors over to FIELD_GET/FIELD_PREP
  KVM: arm64: vgic-v3: Make the userspace accessors use sysreg API
  KVM: arm64: vgic-v3: Push user access into vgic_v3_cpu_sysregs_uaccess()
  KVM: arm64: vgic-v3: Simplify vgic_v3_has_cpu_sysregs_attr()
  KVM: arm64: Get rid of reg_from/to_user()
  KVM: arm64: Consolidate sysreg userspace accesses
  KVM: arm64: Rely on index_to_param() for size checks on userspace access
  KVM: arm64: Introduce generic get_user/set_user helpers for system registers
  KVM: arm64: Reorder handling of invariant sysregs from userspace
  KVM: arm64: Add get_reg_by_id() as a sys_reg_desc retrieving helper

Signed-off-by: default avatarMarc Zyngier <maz@kernel.org>
parents aeb7942b 4274d427
Loading
Loading
Loading
Loading
+0 −2
Original line number Diff line number Diff line
@@ -714,8 +714,6 @@ int kvm_arm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg);

unsigned long kvm_arm_num_sys_reg_descs(struct kvm_vcpu *vcpu);
int kvm_arm_copy_sys_reg_indices(struct kvm_vcpu *vcpu, u64 __user *uindices);
int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);

int __kvm_arm_vcpu_get_events(struct kvm_vcpu *vcpu,
			      struct kvm_vcpu_events *events);
+2 −9
Original line number Diff line number Diff line
@@ -1420,18 +1420,11 @@ void kvm_arch_flush_remote_tlbs_memslot(struct kvm *kvm,
static int kvm_vm_ioctl_set_device_addr(struct kvm *kvm,
					struct kvm_arm_device_addr *dev_addr)
{
	unsigned long dev_id, type;

	dev_id = (dev_addr->id & KVM_ARM_DEVICE_ID_MASK) >>
		KVM_ARM_DEVICE_ID_SHIFT;
	type = (dev_addr->id & KVM_ARM_DEVICE_TYPE_MASK) >>
		KVM_ARM_DEVICE_TYPE_SHIFT;

	switch (dev_id) {
	switch (FIELD_GET(KVM_ARM_DEVICE_ID_MASK, dev_addr->id)) {
	case KVM_ARM_DEVICE_VGIC_V2:
		if (!vgic_present)
			return -ENXIO;
		return kvm_vgic_addr(kvm, type, &dev_addr->addr, true);
		return kvm_set_legacy_vgic_v2_addr(kvm, dev_addr);
	default:
		return -ENODEV;
	}
+114 −168
Original line number Diff line number Diff line
@@ -34,18 +34,11 @@
#include "trace.h"

/*
 * All of this file is extremely similar to the ARM coproc.c, but the
 * types are different. My gut feeling is that it should be pretty
 * easy to merge, but that would be an ABI breakage -- again. VFP
 * would also need to be abstracted.
 *
 * For AArch32, we only take care of what is being trapped. Anything
 * that has to do with init and userspace access has to go via the
 * 64bit interface.
 */

static int reg_from_user(u64 *val, const void __user *uaddr, u64 id);
static int reg_to_user(void __user *uaddr, const u64 *val, u64 id);
static u64 sys_reg_to_index(const struct sys_reg_desc *reg);

static bool read_from_write_only(struct kvm_vcpu *vcpu,
@@ -321,16 +314,8 @@ static bool trap_oslsr_el1(struct kvm_vcpu *vcpu,
}

static int set_oslsr_el1(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
			 const struct kvm_one_reg *reg, void __user *uaddr)
			 u64 val)
{
	u64 id = sys_reg_to_index(rd);
	u64 val;
	int err;

	err = reg_from_user(&val, uaddr, id);
	if (err)
		return err;

	/*
	 * The only modifiable bit is the OSLK bit. Refuse the write if
	 * userspace attempts to change any other bit in the register.
@@ -451,22 +436,16 @@ static bool trap_bvr(struct kvm_vcpu *vcpu,
}

static int set_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];

	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm] = val;
	return 0;
}

static int get_bvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
	const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 *val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];

	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	*val = vcpu->arch.vcpu_debug_state.dbg_bvr[rd->CRm];
	return 0;
}

@@ -493,23 +472,16 @@ static bool trap_bcr(struct kvm_vcpu *vcpu,
}

static int set_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];

	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;

	vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm] = val;
	return 0;
}

static int get_bcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
	const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 *val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];

	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	*val = vcpu->arch.vcpu_debug_state.dbg_bcr[rd->CRm];
	return 0;
}

@@ -537,22 +509,16 @@ static bool trap_wvr(struct kvm_vcpu *vcpu,
}

static int set_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];

	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm] = val;
	return 0;
}

static int get_wvr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
	const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 *val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];

	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	*val = vcpu->arch.vcpu_debug_state.dbg_wvr[rd->CRm];
	return 0;
}

@@ -579,22 +545,16 @@ static bool trap_wcr(struct kvm_vcpu *vcpu,
}

static int set_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];

	if (copy_from_user(r, uaddr, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm] = val;
	return 0;
}

static int get_wcr(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
	const struct kvm_one_reg *reg, void __user *uaddr)
		   u64 *val)
{
	__u64 *r = &vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];

	if (copy_to_user(uaddr, r, KVM_REG_SIZE(reg->id)) != 0)
		return -EFAULT;
	*val = vcpu->arch.vcpu_debug_state.dbg_wcr[rd->CRm];
	return 0;
}

@@ -1227,16 +1187,9 @@ static unsigned int sve_visibility(const struct kvm_vcpu *vcpu,

static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
			       const struct sys_reg_desc *rd,
			       const struct kvm_one_reg *reg, void __user *uaddr)
			       u64 val)
{
	const u64 id = sys_reg_to_index(rd);
	u8 csv2, csv3;
	int err;
	u64 val;

	err = reg_from_user(&val, uaddr, id);
	if (err)
		return err;

	/*
	 * Allow AA64PFR0_EL1.CSV2 to be set from userspace as long as
@@ -1275,27 +1228,17 @@ static int set_id_aa64pfr0_el1(struct kvm_vcpu *vcpu,
 * to be changed.
 */
static int __get_id_reg(const struct kvm_vcpu *vcpu,
			const struct sys_reg_desc *rd, void __user *uaddr,
			const struct sys_reg_desc *rd, u64 *val,
			bool raz)
{
	const u64 id = sys_reg_to_index(rd);
	const u64 val = read_id_reg(vcpu, rd, raz);

	return reg_to_user(uaddr, &val, id);
	*val = read_id_reg(vcpu, rd, raz);
	return 0;
}

static int __set_id_reg(const struct kvm_vcpu *vcpu,
			const struct sys_reg_desc *rd, void __user *uaddr,
			const struct sys_reg_desc *rd, u64 val,
			bool raz)
{
	const u64 id = sys_reg_to_index(rd);
	int err;
	u64 val;

	err = reg_from_user(&val, uaddr, id);
	if (err)
		return err;

	/* This is what we mean by invariant: you can't change it. */
	if (val != read_id_reg(vcpu, rd, raz))
		return -EINVAL;
@@ -1304,47 +1247,37 @@ static int __set_id_reg(const struct kvm_vcpu *vcpu,
}

static int get_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		      const struct kvm_one_reg *reg, void __user *uaddr)
		      u64 *val)
{
	bool raz = sysreg_visible_as_raz(vcpu, rd);

	return __get_id_reg(vcpu, rd, uaddr, raz);
	return __get_id_reg(vcpu, rd, val, raz);
}

static int set_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		      const struct kvm_one_reg *reg, void __user *uaddr)
		      u64 val)
{
	bool raz = sysreg_visible_as_raz(vcpu, rd);

	return __set_id_reg(vcpu, rd, uaddr, raz);
	return __set_id_reg(vcpu, rd, val, raz);
}

static int set_raz_id_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
			  const struct kvm_one_reg *reg, void __user *uaddr)
			  u64 val)
{
	return __set_id_reg(vcpu, rd, uaddr, true);
	return __set_id_reg(vcpu, rd, val, true);
}

static int get_raz_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		       const struct kvm_one_reg *reg, void __user *uaddr)
		       u64 *val)
{
	const u64 id = sys_reg_to_index(rd);
	const u64 val = 0;

	return reg_to_user(uaddr, &val, id);
	*val = 0;
	return 0;
}

static int set_wi_reg(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
		      const struct kvm_one_reg *reg, void __user *uaddr)
		      u64 val)
{
	int err;
	u64 val;

	/* Perform the access even if we are going to ignore the value */
	err = reg_from_user(&val, uaddr, sys_reg_to_index(rd));
	if (err)
		return err;

	return 0;
}

@@ -2639,35 +2572,34 @@ static bool index_to_params(u64 id, struct sys_reg_params *params)
	}
}

const struct sys_reg_desc *find_reg_by_id(u64 id,
					  struct sys_reg_params *params,
const struct sys_reg_desc *get_reg_by_id(u64 id,
					 const struct sys_reg_desc table[],
					 unsigned int num)
{
	if (!index_to_params(id, params))
	struct sys_reg_params params;

	if (!index_to_params(id, &params))
		return NULL;

	return find_reg(params, table, num);
	return find_reg(&params, table, num);
}

/* Decode an index value, and find the sys_reg_desc entry. */
static const struct sys_reg_desc *index_to_sys_reg_desc(struct kvm_vcpu *vcpu,
						    u64 id)
static const struct sys_reg_desc *
id_to_sys_reg_desc(struct kvm_vcpu *vcpu, u64 id,
		   const struct sys_reg_desc table[], unsigned int num)

{
	const struct sys_reg_desc *r;
	struct sys_reg_params params;

	/* We only do sys_reg for now. */
	if ((id & KVM_REG_ARM_COPROC_MASK) != KVM_REG_ARM64_SYSREG)
		return NULL;

	if (!index_to_params(id, &params))
		return NULL;

	r = find_reg(&params, sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
	r = get_reg_by_id(id, table, num);

	/* Not saved in the sys_reg array and not otherwise accessible? */
	if (r && !(r->reg || r->get_user))
	if (r && (!(r->reg || r->get_user) || sysreg_hidden(vcpu, r)))
		r = NULL;

	return r;
@@ -2707,48 +2639,30 @@ static struct sys_reg_desc invariant_sys_regs[] = {
	{ SYS_DESC(SYS_CTR_EL0), NULL, get_ctr_el0 },
};

static int reg_from_user(u64 *val, const void __user *uaddr, u64 id)
static int get_invariant_sys_reg(u64 id, u64 __user *uaddr)
{
	if (copy_from_user(val, uaddr, KVM_REG_SIZE(id)) != 0)
		return -EFAULT;
	return 0;
}

static int reg_to_user(void __user *uaddr, const u64 *val, u64 id)
{
	if (copy_to_user(uaddr, val, KVM_REG_SIZE(id)) != 0)
		return -EFAULT;
	return 0;
}

static int get_invariant_sys_reg(u64 id, void __user *uaddr)
{
	struct sys_reg_params params;
	const struct sys_reg_desc *r;

	r = find_reg_by_id(id, &params, invariant_sys_regs,
	r = get_reg_by_id(id, invariant_sys_regs,
			  ARRAY_SIZE(invariant_sys_regs));
	if (!r)
		return -ENOENT;

	return reg_to_user(uaddr, &r->val, id);
	return put_user(r->val, uaddr);
}

static int set_invariant_sys_reg(u64 id, void __user *uaddr)
static int set_invariant_sys_reg(u64 id, u64 __user *uaddr)
{
	struct sys_reg_params params;
	const struct sys_reg_desc *r;
	int err;
	u64 val = 0; /* Make sure high bits are 0 for 32-bit regs */
	u64 val;

	r = find_reg_by_id(id, &params, invariant_sys_regs,
	r = get_reg_by_id(id, invariant_sys_regs,
			  ARRAY_SIZE(invariant_sys_regs));
	if (!r)
		return -ENOENT;

	err = reg_from_user(&val, uaddr, id);
	if (err)
		return err;
	if (get_user(val, uaddr))
		return -EFAULT;

	/* This is what we mean by invariant: you can't change it. */
	if (r->val != val)
@@ -2839,54 +2753,86 @@ static int demux_c15_set(u64 id, void __user *uaddr)
	}
}

int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
			 const struct sys_reg_desc table[], unsigned int num)
{
	u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
	const struct sys_reg_desc *r;
	u64 val;
	int ret;

	r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
	if (!r)
		return -ENOENT;

	if (r->get_user) {
		ret = (r->get_user)(vcpu, r, &val);
	} else {
		val = __vcpu_sys_reg(vcpu, r->reg);
		ret = 0;
	}

	if (!ret)
		ret = put_user(val, uaddr);

	return ret;
}

int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
	int err;

	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
		return demux_c15_get(reg->id, uaddr);

	if (KVM_REG_SIZE(reg->id) != sizeof(__u64))
		return -ENOENT;
	err = get_invariant_sys_reg(reg->id, uaddr);
	if (err != -ENOENT)
		return err;

	r = index_to_sys_reg_desc(vcpu, reg->id);
	if (!r)
		return get_invariant_sys_reg(reg->id, uaddr);
	return kvm_sys_reg_get_user(vcpu, reg,
				    sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
}

	/* Check for regs disabled by runtime config */
	if (sysreg_hidden(vcpu, r))
int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
			 const struct sys_reg_desc table[], unsigned int num)
{
	u64 __user *uaddr = (u64 __user *)(unsigned long)reg->addr;
	const struct sys_reg_desc *r;
	u64 val;
	int ret;

	if (get_user(val, uaddr))
		return -EFAULT;

	r = id_to_sys_reg_desc(vcpu, reg->id, table, num);
	if (!r)
		return -ENOENT;

	if (r->get_user)
		return (r->get_user)(vcpu, r, reg, uaddr);
	if (r->set_user) {
		ret = (r->set_user)(vcpu, r, val);
	} else {
		__vcpu_sys_reg(vcpu, r->reg) = val;
		ret = 0;
	}

	return reg_to_user(uaddr, &__vcpu_sys_reg(vcpu, r->reg), reg->id);
	return ret;
}

int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
{
	const struct sys_reg_desc *r;
	void __user *uaddr = (void __user *)(unsigned long)reg->addr;
	int err;

	if ((reg->id & KVM_REG_ARM_COPROC_MASK) == KVM_REG_ARM_DEMUX)
		return demux_c15_set(reg->id, uaddr);

	if (KVM_REG_SIZE(reg->id) != sizeof(__u64))
		return -ENOENT;

	r = index_to_sys_reg_desc(vcpu, reg->id);
	if (!r)
		return set_invariant_sys_reg(reg->id, uaddr);

	/* Check for regs disabled by runtime config */
	if (sysreg_hidden(vcpu, r))
		return -ENOENT;

	if (r->set_user)
		return (r->set_user)(vcpu, r, reg, uaddr);
	err = set_invariant_sys_reg(reg->id, uaddr);
	if (err != -ENOENT)
		return err;

	return reg_from_user(&__vcpu_sys_reg(vcpu, r->reg), uaddr, reg->id);
	return kvm_sys_reg_set_user(vcpu, reg,
				    sys_reg_descs, ARRAY_SIZE(sys_reg_descs));
}

static unsigned int num_demux_regs(void)
+12 −6
Original line number Diff line number Diff line
@@ -75,9 +75,9 @@ struct sys_reg_desc {

	/* Custom get/set_user functions, fallback to generic if NULL */
	int (*get_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
			const struct kvm_one_reg *reg, void __user *uaddr);
			u64 *val);
	int (*set_user)(struct kvm_vcpu *vcpu, const struct sys_reg_desc *rd,
			const struct kvm_one_reg *reg, void __user *uaddr);
			u64 val);

	/* Return mask of REG_* runtime visibility overrides */
	unsigned int (*visibility)(const struct kvm_vcpu *vcpu,
@@ -190,11 +190,17 @@ find_reg(const struct sys_reg_params *params, const struct sys_reg_desc table[],
	return __inline_bsearch((void *)pval, table, num, sizeof(table[0]), match_sys_reg);
}

const struct sys_reg_desc *find_reg_by_id(u64 id,
					  struct sys_reg_params *params,
const struct sys_reg_desc *get_reg_by_id(u64 id,
					 const struct sys_reg_desc table[],
					 unsigned int num);

int kvm_arm_sys_reg_get_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
int kvm_arm_sys_reg_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *);
int kvm_sys_reg_get_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
			 const struct sys_reg_desc table[], unsigned int num);
int kvm_sys_reg_set_user(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg,
			 const struct sys_reg_desc table[], unsigned int num);

#define AA32(_x)	.aarch32_map = AA32_##_x
#define Op0(_x) 	.Op0 = _x
#define Op1(_x) 	.Op1 = _x
+263 −199

File changed.

Preview size limit exceeded, changes collapsed.

Loading