linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PULL 0/8] KVM/ARM Fixes for v4.14
@ 2017-10-30  2:55 Christoffer Dall
  2017-10-30  2:55 ` [PULL 1/8] KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table Christoffer Dall
                   ` (8 more replies)
  0 siblings, 9 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

Hi Paolo and Radim,

Here are the fixes we've been trying to get ready for v4.14 for a while;
sorry about sending these so late.

We're fixing:

 - a number of issues with saving/restoring the ITS
 - a bug in KVM/ARM when branch profiling is enabled in Hyp mode
 - an emulation bug for 32-bit guests when injecting aborts
 - a failure to check if a kmalloc succeeds in the ITS emulation

The following changes since commit 8a5776a5f49812d29fe4b2d0a2d71675c3facf3f:

  Linux 4.14-rc4 (2017-10-08 20:53:29 -0700)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-fixes-for-v4.14

for you to fetch changes up to c2385eaa6c5a87cdc4e04ed589ae103ca3297c84:

  KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables (2017-10-29 03:25:06 +0100)


Thanks,
-Christoffer

Christoffer Dall (1):
      KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table

Dongjiu Geng (1):
      arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort

Eric Auger (3):
      KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value
      KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS
      KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables

Julien Thierry (2):
      arm/arm64: kvm: Move initialization completion message
      arm/arm64: kvm: Disable branch profiling in HYP code

wanghaibin (1):
      KVM: arm/arm64: vgic-its: Fix return value for device table restore

 arch/arm/kvm/emulate.c        |  6 ++--
 arch/arm/kvm/hyp/Makefile     |  2 +-
 arch/arm64/kvm/hyp/Makefile   |  2 +-
 arch/arm64/kvm/inject_fault.c | 16 +++++++++-
 virt/kvm/arm/arm.c            | 31 +++++++++---------
 virt/kvm/arm/vgic/vgic-its.c  | 73 ++++++++++++++++++++++++++++---------------
 6 files changed, 81 insertions(+), 49 deletions(-)

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PULL 1/8] KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
@ 2017-10-30  2:55 ` Christoffer Dall
  2017-10-30  2:55 ` [PULL 2/8] arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort Christoffer Dall
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

We currently allocate an entry dynamically, but we never check if the
allocation actually succeeded.  We actually don't need a dynamic
allocation, because we know the maximum size of an ITS table entry, so
we can simply use an allocation on the stack.

Cc: <stable@vger.kernel.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index f51c1e1b3f70..77652885a7c1 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1801,37 +1801,33 @@ typedef int (*entry_fn_t)(struct vgic_its *its, u32 id, void *entry,
 static int scan_its_table(struct vgic_its *its, gpa_t base, int size, int esz,
 			  int start_id, entry_fn_t fn, void *opaque)
 {
-	void *entry = kzalloc(esz, GFP_KERNEL);
 	struct kvm *kvm = its->dev->kvm;
 	unsigned long len = size;
 	int id = start_id;
 	gpa_t gpa = base;
+	char entry[esz];
 	int ret;
 
+	memset(entry, 0, esz);
+
 	while (len > 0) {
 		int next_offset;
 		size_t byte_offset;
 
 		ret = kvm_read_guest(kvm, gpa, entry, esz);
 		if (ret)
-			goto out;
+			return ret;
 
 		next_offset = fn(its, id, entry, opaque);
-		if (next_offset <= 0) {
-			ret = next_offset;
-			goto out;
-		}
+		if (next_offset <= 0)
+			return next_offset;
 
 		byte_offset = next_offset * esz;
 		id += next_offset;
 		gpa += byte_offset;
 		len -= byte_offset;
 	}
-	ret =  1;
-
-out:
-	kfree(entry);
-	return ret;
+	return 1;
 }
 
 /**
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 2/8] arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
  2017-10-30  2:55 ` [PULL 1/8] KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table Christoffer Dall
@ 2017-10-30  2:55 ` Christoffer Dall
  2017-10-30  2:55 ` [PULL 3/8] arm/arm64: kvm: Move initialization completion message Christoffer Dall
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

From: Dongjiu Geng <gengdongjiu@huawei.com>

When a exception is trapped to EL2, hardware uses  ELR_ELx to hold
the current fault instruction address. If KVM wants to inject a
abort to 32 bit guest, it needs to set the LR register for the
guest to emulate this abort happened in the guest. Because ARM32
architecture is pipelined execution, so the LR value has an offset to
the fault instruction address.

The offsets applied to Link value for exceptions as shown below,
which should be added for the ARM32 link register(LR).

Table taken from ARMv8 ARM DDI0487B-B, table G1-10:
Exception			Offset, for PE state of:
				A32 	  T32
Undefined Instruction 		+4 	  +2
Prefetch Abort 			+4 	  +4
Data Abort 			+8 	  +8
IRQ or FIQ 			+4 	  +4

  [ Removed unused variables in inject_abt to avoid compile warnings.
    -- Christoffer ]

Cc: <stable@vger.kernel.org>
Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com>
Tested-by: Haibin Zhang <zhanghaibin7@huawei.com>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <cdall@linaro.org>
---
 arch/arm/kvm/emulate.c        |  6 ++----
 arch/arm64/kvm/inject_fault.c | 16 +++++++++++++++-
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 0064b86a2c87..30a13647c54c 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -227,7 +227,7 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu)
 	u32 return_offset = (is_thumb) ? 2 : 4;
 
 	kvm_update_psr(vcpu, UND_MODE);
-	*vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) - return_offset;
+	*vcpu_reg(vcpu, 14) = *vcpu_pc(vcpu) + return_offset;
 
 	/* Branch to exception vector */
 	*vcpu_pc(vcpu) = exc_vector_base(vcpu) + vect_offset;
@@ -239,10 +239,8 @@ void kvm_inject_undefined(struct kvm_vcpu *vcpu)
  */
 static void inject_abt(struct kvm_vcpu *vcpu, bool is_pabt, unsigned long addr)
 {
-	unsigned long cpsr = *vcpu_cpsr(vcpu);
-	bool is_thumb = (cpsr & PSR_T_BIT);
 	u32 vect_offset;
-	u32 return_offset = (is_thumb) ? 4 : 0;
+	u32 return_offset = (is_pabt) ? 4 : 8;
 	bool is_lpae;
 
 	kvm_update_psr(vcpu, ABT_MODE);
diff --git a/arch/arm64/kvm/inject_fault.c b/arch/arm64/kvm/inject_fault.c
index da6a8cfa54a0..3556715a774e 100644
--- a/arch/arm64/kvm/inject_fault.c
+++ b/arch/arm64/kvm/inject_fault.c
@@ -33,12 +33,26 @@
 #define LOWER_EL_AArch64_VECTOR		0x400
 #define LOWER_EL_AArch32_VECTOR		0x600
 
+/*
+ * Table taken from ARMv8 ARM DDI0487B-B, table G1-10.
+ */
+static const u8 return_offsets[8][2] = {
+	[0] = { 0, 0 },		/* Reset, unused */
+	[1] = { 4, 2 },		/* Undefined */
+	[2] = { 0, 0 },		/* SVC, unused */
+	[3] = { 4, 4 },		/* Prefetch abort */
+	[4] = { 8, 8 },		/* Data abort */
+	[5] = { 0, 0 },		/* HVC, unused */
+	[6] = { 4, 4 },		/* IRQ, unused */
+	[7] = { 4, 4 },		/* FIQ, unused */
+};
+
 static void prepare_fault32(struct kvm_vcpu *vcpu, u32 mode, u32 vect_offset)
 {
 	unsigned long cpsr;
 	unsigned long new_spsr_value = *vcpu_cpsr(vcpu);
 	bool is_thumb = (new_spsr_value & COMPAT_PSR_T_BIT);
-	u32 return_offset = (is_thumb) ? 4 : 0;
+	u32 return_offset = return_offsets[vect_offset >> 2][is_thumb];
 	u32 sctlr = vcpu_cp15(vcpu, c1_SCTLR);
 
 	cpsr = mode | COMPAT_PSR_I_BIT;
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 3/8] arm/arm64: kvm: Move initialization completion message
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
  2017-10-30  2:55 ` [PULL 1/8] KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table Christoffer Dall
  2017-10-30  2:55 ` [PULL 2/8] arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort Christoffer Dall
@ 2017-10-30  2:55 ` Christoffer Dall
  2017-10-30  2:55 ` [PULL 4/8] arm/arm64: kvm: Disable branch profiling in HYP code Christoffer Dall
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

From: Julien Thierry <julien.thierry@arm.com>

KVM is being a bit too optimistic, Hyp mode is said to be initialized
when Hyp segments have only been mapped.

Notify KVM's successful initialization only once it is really fully
initialized.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/arm.c | 31 ++++++++++++++-----------------
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
index b9f68e4add71..95cba0799828 100644
--- a/virt/kvm/arm/arm.c
+++ b/virt/kvm/arm/arm.c
@@ -1326,21 +1326,12 @@ static void teardown_hyp_mode(void)
 {
 	int cpu;
 
-	if (is_kernel_in_hyp_mode())
-		return;
-
 	free_hyp_pgds();
 	for_each_possible_cpu(cpu)
 		free_page(per_cpu(kvm_arm_hyp_stack_page, cpu));
 	hyp_cpu_pm_exit();
 }
 
-static int init_vhe_mode(void)
-{
-	kvm_info("VHE mode initialized successfully\n");
-	return 0;
-}
-
 /**
  * Inits Hyp-mode on all online CPUs
  */
@@ -1421,8 +1412,6 @@ static int init_hyp_mode(void)
 		}
 	}
 
-	kvm_info("Hyp mode initialized successfully\n");
-
 	return 0;
 
 out_err:
@@ -1456,6 +1445,7 @@ int kvm_arch_init(void *opaque)
 {
 	int err;
 	int ret, cpu;
+	bool in_hyp_mode;
 
 	if (!is_hyp_mode_available()) {
 		kvm_err("HYP mode not available\n");
@@ -1474,21 +1464,28 @@ int kvm_arch_init(void *opaque)
 	if (err)
 		return err;
 
-	if (is_kernel_in_hyp_mode())
-		err = init_vhe_mode();
-	else
+	in_hyp_mode = is_kernel_in_hyp_mode();
+
+	if (!in_hyp_mode) {
 		err = init_hyp_mode();
-	if (err)
-		goto out_err;
+		if (err)
+			goto out_err;
+	}
 
 	err = init_subsystems();
 	if (err)
 		goto out_hyp;
 
+	if (in_hyp_mode)
+		kvm_info("VHE mode initialized successfully\n");
+	else
+		kvm_info("Hyp mode initialized successfully\n");
+
 	return 0;
 
 out_hyp:
-	teardown_hyp_mode();
+	if (!in_hyp_mode)
+		teardown_hyp_mode();
 out_err:
 	teardown_common_resources();
 	return err;
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 4/8] arm/arm64: kvm: Disable branch profiling in HYP code
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (2 preceding siblings ...)
  2017-10-30  2:55 ` [PULL 3/8] arm/arm64: kvm: Move initialization completion message Christoffer Dall
@ 2017-10-30  2:55 ` Christoffer Dall
  2017-10-30  2:56 ` [PULL 5/8] KVM: arm/arm64: vgic-its: Fix return value for device table restore Christoffer Dall
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:55 UTC (permalink / raw)
  To: linux-arm-kernel

From: Julien Thierry <julien.thierry@arm.com>

When HYP code runs into branch profiling code, it attempts to jump to
unmapped memory, causing a HYP Panic.

Disable the branch profiling for code designed to run at HYP mode.

Signed-off-by: Julien Thierry <julien.thierry@arm.com>
Acked-by: Marc Zyngier <marc.zyngier@arm.com>
Cc: Christoffer Dall <christoffer.dall@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: <stable@vger.kernel.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/hyp/Makefile   | 2 +-
 arch/arm64/kvm/hyp/Makefile | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 8679405b0b2b..92eab1d51785 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -2,7 +2,7 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
-ccflags-y += -fno-stack-protector
+ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
 
 KVM=../../../../virt/kvm
 
diff --git a/arch/arm64/kvm/hyp/Makefile b/arch/arm64/kvm/hyp/Makefile
index 14c4e3b14bcb..48b03547a969 100644
--- a/arch/arm64/kvm/hyp/Makefile
+++ b/arch/arm64/kvm/hyp/Makefile
@@ -2,7 +2,7 @@
 # Makefile for Kernel-based Virtual Machine module, HYP part
 #
 
-ccflags-y += -fno-stack-protector
+ccflags-y += -fno-stack-protector -DDISABLE_BRANCH_PROFILING
 
 KVM=../../../../virt/kvm
 
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 5/8] KVM: arm/arm64: vgic-its: Fix return value for device table restore
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (3 preceding siblings ...)
  2017-10-30  2:55 ` [PULL 4/8] arm/arm64: kvm: Disable branch profiling in HYP code Christoffer Dall
@ 2017-10-30  2:56 ` Christoffer Dall
  2017-10-30  2:56 ` [PULL 6/8] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value Christoffer Dall
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:56 UTC (permalink / raw)
  To: linux-arm-kernel

From: wanghaibin <wanghaibin.wang@huawei.com>

If ITT only contains invalid entries, vgic_its_restore_itt
returns 1 and this is considered as an an error in
vgic_its_restore_dte.

Also in case the device table only contains invalid entries,
the table restore fails and this is not correct.

This patch fixes those 2 issues:
- vgic_its_restore_itt now returns <= 0 values. If all
  ITEs are invalid, this is considered as successful.
- vgic_its_restore_device_tables also returns <= 0 values.

We also simplify the returned value computation in
handle_l1_dte.

Signed-off-by: wanghaibin <wanghaibin.wang@huawei.com>
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 77652885a7c1..76685f4c6261 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1936,6 +1936,14 @@ static int vgic_its_save_itt(struct vgic_its *its, struct its_device *device)
 	return 0;
 }
 
+/**
+ * vgic_its_restore_itt - restore the ITT of a device
+ *
+ * @its: its handle
+ * @dev: device handle
+ *
+ * Return 0 on success, < 0 on error
+ */
 static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
@@ -1947,6 +1955,10 @@ static int vgic_its_restore_itt(struct vgic_its *its, struct its_device *dev)
 	ret = scan_its_table(its, base, max_size, ite_esz, 0,
 			     vgic_its_restore_ite, dev);
 
+	/* scan_its_table returns +1 if all ITEs are invalid */
+	if (ret > 0)
+		ret = 0;
+
 	return ret;
 }
 
@@ -2103,10 +2115,7 @@ static int handle_l1_dte(struct vgic_its *its, u32 id, void *addr,
 	ret = scan_its_table(its, gpa, SZ_64K, dte_esz,
 			     l2_start_id, vgic_its_restore_dte, NULL);
 
-	if (ret <= 0)
-		return ret;
-
-	return 1;
+	return ret;
 }
 
 /**
@@ -2136,8 +2145,9 @@ static int vgic_its_restore_device_tables(struct vgic_its *its)
 				     vgic_its_restore_dte, NULL);
 	}
 
+	/* scan_its_table returns +1 if all entries are invalid */
 	if (ret > 0)
-		ret = -EINVAL;
+		ret = 0;
 
 	return ret;
 }
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 6/8] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (4 preceding siblings ...)
  2017-10-30  2:56 ` [PULL 5/8] KVM: arm/arm64: vgic-its: Fix return value for device table restore Christoffer Dall
@ 2017-10-30  2:56 ` Christoffer Dall
  2017-10-30  2:56 ` [PULL 7/8] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS Christoffer Dall
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:56 UTC (permalink / raw)
  To: linux-arm-kernel

From: Eric Auger <eric.auger@redhat.com>

vgic_its_restore_cte returns +1 if the collection table entry
is valid and properly decoded. As a consequence, if the
collection table is fully filled with valid data that are
decoded without error, vgic_its_restore_collection_table()
returns +1. This is wrong.

Let's return 0 in that case.

Fixes: ea1ad53e1e31a3 (KVM: arm64: vgic-its: Collection table save/restore)
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 76685f4c6261..6a715a6ec64e 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2264,6 +2264,10 @@ static int vgic_its_restore_collection_table(struct vgic_its *its)
 		gpa += cte_esz;
 		read += cte_esz;
 	}
+
+	if (ret > 0)
+		return 0;
+
 	return ret;
 }
 
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 7/8] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (5 preceding siblings ...)
  2017-10-30  2:56 ` [PULL 6/8] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value Christoffer Dall
@ 2017-10-30  2:56 ` Christoffer Dall
  2017-10-30  2:56 ` [PULL 8/8] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables Christoffer Dall
  2017-11-02 17:38 ` [PULL 0/8] KVM/ARM Fixes for v4.14 Paolo Bonzini
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:56 UTC (permalink / raw)
  To: linux-arm-kernel

From: Eric Auger <eric.auger@redhat.com>

The spec says it is UNPREDICTABLE to enable the ITS
if any of the following conditions are true:

- GITS_CBASER.Valid == 0.
- GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
  where the Type field indicates Device.
- GITS_BASER<n>.Valid == 0, for any GITS_BASER<n> register
  where the Type field indicates Interrupt Collection and
  GITS_TYPER.HCC == 0.

In that case, let's keep the ITS disabled.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reported-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 6a715a6ec64e..e69ef7d27fde 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -1466,6 +1466,16 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 {
 	mutex_lock(&its->cmd_lock);
 
+	/*
+	 * It is UNPREDICTABLE to enable the ITS if any of the CBASER or
+	 * device/collection BASER are invalid
+	 */
+	if (!its->enabled && (val & GITS_CTLR_ENABLE) &&
+		(!(its->baser_device_table & GITS_BASER_VALID) ||
+		 !(its->baser_coll_table & GITS_BASER_VALID) ||
+		 !(its->cbaser & GITS_CBASER_VALID)))
+		goto out;
+
 	its->enabled = !!(val & GITS_CTLR_ENABLE);
 
 	/*
@@ -1474,6 +1484,7 @@ static void vgic_mmio_write_its_ctlr(struct kvm *kvm, struct vgic_its *its,
 	 */
 	vgic_its_process_commands(kvm, its);
 
+out:
 	mutex_unlock(&its->cmd_lock);
 }
 
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 8/8] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (6 preceding siblings ...)
  2017-10-30  2:56 ` [PULL 7/8] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS Christoffer Dall
@ 2017-10-30  2:56 ` Christoffer Dall
  2017-11-02 17:38 ` [PULL 0/8] KVM/ARM Fixes for v4.14 Paolo Bonzini
  8 siblings, 0 replies; 10+ messages in thread
From: Christoffer Dall @ 2017-10-30  2:56 UTC (permalink / raw)
  To: linux-arm-kernel

From: Eric Auger <eric.auger@redhat.com>

At the moment we don't properly check the GITS_BASER<n>.Valid
bit before saving the collection and device tables.

On vgic_its_save_collection_table() we use the GITS_BASER gpa
field whereas the Valid bit should be used.

On vgic_its_save_device_tables() there is no check. This can
cause various bugs, among which a subsequent fault when accessing
the table in guest memory.

Let's systematically check the Valid bit before doing anything.

We also uniformize the code between save and restore.

Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>
Reviewed-by: Marc Zyngier <marc.zyngier@arm.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 virt/kvm/arm/vgic/vgic-its.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index e69ef7d27fde..547f12dc4d54 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -2067,11 +2067,12 @@ static int vgic_its_device_cmp(void *priv, 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;
-	u64 baser;
 
-	baser = its->baser_device_table;
+	if (!(baser & GITS_BASER_VALID))
+		return 0;
 
 	list_sort(NULL, &its->device_list, vgic_its_device_cmp);
 
@@ -2215,17 +2216,17 @@ static int vgic_its_restore_cte(struct vgic_its *its, gpa_t gpa, int esz)
 static int vgic_its_save_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
+	gpa_t gpa = BASER_ADDRESS(baser);
 	struct its_collection *collection;
 	u64 val;
-	gpa_t gpa;
 	size_t max_size, filled = 0;
 	int ret, cte_esz = abi->cte_esz;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
-	if (!gpa)
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	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);
@@ -2256,17 +2257,18 @@ static int vgic_its_save_collection_table(struct vgic_its *its)
 static int vgic_its_restore_collection_table(struct vgic_its *its)
 {
 	const struct vgic_its_abi *abi = vgic_its_get_abi(its);
+	u64 baser = its->baser_coll_table;
 	int cte_esz = abi->cte_esz;
 	size_t max_size, read = 0;
 	gpa_t gpa;
 	int ret;
 
-	if (!(its->baser_coll_table & GITS_BASER_VALID))
+	if (!(baser & GITS_BASER_VALID))
 		return 0;
 
-	gpa = BASER_ADDRESS(its->baser_coll_table);
+	gpa = BASER_ADDRESS(baser);
 
-	max_size = GITS_BASER_NR_PAGES(its->baser_coll_table) * SZ_64K;
+	max_size = GITS_BASER_NR_PAGES(baser) * SZ_64K;
 
 	while (read < max_size) {
 		ret = vgic_its_restore_cte(its, gpa, cte_esz);
-- 
2.14.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PULL 0/8] KVM/ARM Fixes for v4.14
  2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
                   ` (7 preceding siblings ...)
  2017-10-30  2:56 ` [PULL 8/8] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables Christoffer Dall
@ 2017-11-02 17:38 ` Paolo Bonzini
  8 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2017-11-02 17:38 UTC (permalink / raw)
  To: linux-arm-kernel

On 30/10/2017 03:55, Christoffer Dall wrote:
> Hi Paolo and Radim,
> 
> Here are the fixes we've been trying to get ready for v4.14 for a while;
> sorry about sending these so late.
> 
> We're fixing:
> 
>  - a number of issues with saving/restoring the ITS
>  - a bug in KVM/ARM when branch profiling is enabled in Hyp mode
>  - an emulation bug for 32-bit guests when injecting aborts
>  - a failure to check if a kmalloc succeeds in the ITS emulation
> 
> The following changes since commit 8a5776a5f49812d29fe4b2d0a2d71675c3facf3f:
> 
>   Linux 4.14-rc4 (2017-10-08 20:53:29 -0700)
> 
> are available in the git repository at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git tags/kvm-arm-fixes-for-v4.14
> 
> for you to fetch changes up to c2385eaa6c5a87cdc4e04ed589ae103ca3297c84:
> 
>   KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables (2017-10-29 03:25:06 +0100)
> 
> 
> Thanks,
> -Christoffer
> 
> Christoffer Dall (1):
>       KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table
> 
> Dongjiu Geng (1):
>       arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort
> 
> Eric Auger (3):
>       KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value
>       KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS
>       KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables
> 
> Julien Thierry (2):
>       arm/arm64: kvm: Move initialization completion message
>       arm/arm64: kvm: Disable branch profiling in HYP code
> 
> wanghaibin (1):
>       KVM: arm/arm64: vgic-its: Fix return value for device table restore
> 
>  arch/arm/kvm/emulate.c        |  6 ++--
>  arch/arm/kvm/hyp/Makefile     |  2 +-
>  arch/arm64/kvm/hyp/Makefile   |  2 +-
>  arch/arm64/kvm/inject_fault.c | 16 +++++++++-
>  virt/kvm/arm/arm.c            | 31 +++++++++---------
>  virt/kvm/arm/vgic/vgic-its.c  | 73 ++++++++++++++++++++++++++++---------------
>  6 files changed, 81 insertions(+), 49 deletions(-)
> 


Pulled, thanks.

Paolo

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-11-02 17:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-30  2:55 [PULL 0/8] KVM/ARM Fixes for v4.14 Christoffer Dall
2017-10-30  2:55 ` [PULL 1/8] KVM: arm64: its: Fix missing dynamic allocation check in scan_its_table Christoffer Dall
2017-10-30  2:55 ` [PULL 2/8] arm/arm64: KVM: set right LR register value for 32 bit guest when inject abort Christoffer Dall
2017-10-30  2:55 ` [PULL 3/8] arm/arm64: kvm: Move initialization completion message Christoffer Dall
2017-10-30  2:55 ` [PULL 4/8] arm/arm64: kvm: Disable branch profiling in HYP code Christoffer Dall
2017-10-30  2:56 ` [PULL 5/8] KVM: arm/arm64: vgic-its: Fix return value for device table restore Christoffer Dall
2017-10-30  2:56 ` [PULL 6/8] KVM: arm/arm64: vgic-its: Fix vgic_its_restore_collection_table returned value Christoffer Dall
2017-10-30  2:56 ` [PULL 7/8] KVM: arm/arm64: vgic-its: Check CBASER/BASER validity before enabling the ITS Christoffer Dall
2017-10-30  2:56 ` [PULL 8/8] KVM: arm/arm64: vgic-its: Check GITS_BASER Valid bit before saving tables Christoffer Dall
2017-11-02 17:38 ` [PULL 0/8] KVM/ARM Fixes for v4.14 Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).