[PATCH 0/7] KVM/ARM fixes for 4.15-rc1

[PATCH 0/7] KVM/ARM fixes for 4.15-rc1

[Marc Zyngier]

Hi Christoffer,

Here's a handful of KVM/ARM fixes I've collected over the week:

- Four bugs reported by AKASHI Takahiro who ran the SMATCH static
  analyser on the KVM/ARM code. All great findings! I've split the
  fixes so that they can be backported individually.
- A nice corner case found by Kristina Martsenko on arm64 (which she
  posted separately), which  actually originated from the 32bit port
  (and I've now included a patch plugging that one as well).
- One GICv4 leftover.

I've stashed all of that on my kvm-arm64/fixes-4.15 branch.

Thanks,

	M.

Kristina Martsenko (1):
  arm64: KVM: fix VTTBR_BADDR_MASK BUG_ON off-by-one

Marc Zyngier (6):
  KVM: arm/arm64: vgic-irqfd: Fix MSI entry allocation
  KVM: arm/arm64: vgic: Preserve the revious read from the pending table
  KVM: arm/arm64: vgic-its: Preserve the revious read from the pending
    table
  KVM: arm/arm64: vgic-its: Check result of allocation before use
  KVM: arm/arm64: vgic-v4: Only perform an unmap for valid vLPIs
  arm: KVM: Fix VTTBR_BADDR_MASK BUG_ON off-by-one

 arch/arm/include/asm/kvm_arm.h   | 3 +--
 arch/arm64/include/asm/kvm_arm.h | 3 +--
 virt/kvm/arm/vgic/vgic-irqfd.c   | 3 +--
 virt/kvm/arm/vgic/vgic-its.c     | 4 +++-
 virt/kvm/arm/vgic/vgic-v3.c      | 2 +-
 virt/kvm/arm/vgic/vgic-v4.c      | 6 ++++--
 6 files changed, 11 insertions(+), 10 deletions(-)

-- 
2.14.2

[PATCH 1/7] KVM: arm/arm64: vgic-irqfd: Fix MSI entry allocation

[Marc Zyngier]

Using the size of the structure we're allocating is a good idea
and avoids any surprise... In this case, we're happilly confusing
kvm_kernel_irq_routing_entry and kvm_irq_routing_entry...

Fixes: 95b110ab9a09 ("KVM: arm/arm64: Enable irqchip routing")
Cc: stable at vger.kernel.org # 4.8
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-irqfd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
index b7baf581611a..99e026d2dade 100644
--- a/virt/kvm/arm/vgic/vgic-irqfd.c
+++ b/virt/kvm/arm/vgic/vgic-irqfd.c
@@ -112,8 +112,7 @@ int kvm_vgic_setup_default_irq_routing(struct kvm *kvm)
 	u32 nr = dist->nr_spis;
 	int i, ret;
 
-	entries = kcalloc(nr, sizeof(struct kvm_kernel_irq_routing_entry),
-			  GFP_KERNEL);
+	entries = kcalloc(nr, sizeof(*entries), GFP_KERNEL);
 	if (!entries)
 		return -ENOMEM;
 
-- 
2.14.2

[PATCH 2/7] KVM: arm/arm64: vgic: Preserve the revious read from the pending table

[Marc Zyngier]

The current pending table parsing code assumes that we keep the
previous read of the pending bits, but keep that variable in
the current block, making sure it is discarded on each loop.

We end-up using whatever is on the stack. Who knows, it might
just be the right thing...

Fixes: 280771252c1ba ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
Cc: stable at vger.kernel.org # 4.12
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v3.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
index 2f05f732d3fd..f47e8481fa45 100644
--- a/virt/kvm/arm/vgic/vgic-v3.c
+++ b/virt/kvm/arm/vgic/vgic-v3.c
@@ -327,13 +327,13 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
 	int last_byte_offset = -1;
 	struct vgic_irq *irq;
 	int ret;
+	u8 val;
 
 	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
 		int byte_offset, bit_nr;
 		struct kvm_vcpu *vcpu;
 		gpa_t pendbase, ptr;
 		bool stored;
-		u8 val;
 
 		vcpu = irq->target_vcpu;
 		if (!vcpu)
-- 
2.14.2

[PATCH 3/7] KVM: arm/arm64: vgic-its: Preserve the revious read from the pending table

[Marc Zyngier]

The current pending table parsing code assumes that we keep the
previous read of the pending bits, but keep that variable in
the current block, making sure it is discarded on each loop.

We end-up using whatever is on the stack. Who knows, it might
just be the right thing...

Fixes: 33d3bc9556a7d ("KVM: arm64: vgic-its: Read initial LPI pending table")
Cc: stable at vger.kernel.org # 4.8
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index a3754ec719c4..370086006838 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -421,6 +421,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
 	u32 *intids;
 	int nr_irqs, i;
 	unsigned long flags;
+	u8 pendmask;
 
 	nr_irqs = vgic_copy_lpi_list(vcpu, &intids);
 	if (nr_irqs < 0)
@@ -428,7 +429,6 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
 
 	for (i = 0; i < nr_irqs; i++) {
 		int byte_offset, bit_nr;
-		u8 pendmask;
 
 		byte_offset = intids[i] / BITS_PER_BYTE;
 		bit_nr = intids[i] % BITS_PER_BYTE;
-- 
2.14.2

[PATCH 4/7] KVM: arm/arm64: vgic-its: Check result of allocation before use

[Marc Zyngier]

We miss a test against NULL after allocation.

Fixes: 6d03a68f8054 ("KVM: arm64: vgic-its: Turn device_id validation into generic ID validation")
Cc: stable at vger.kernel.org # 4.8
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-its.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
index 370086006838..30f7c7e6d2f4 100644
--- a/virt/kvm/arm/vgic/vgic-its.c
+++ b/virt/kvm/arm/vgic/vgic-its.c
@@ -821,6 +821,8 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
 		return E_ITS_MAPC_COLLECTION_OOR;
 
 	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
+	if (!collection)
+		return -ENOMEM;
 
 	collection->collection_id = coll_id;
 	collection->target_addr = COLLECTION_NOT_MAPPED;
-- 
2.14.2

[PATCH 5/7] KVM: arm/arm64: vgic-v4: Only perform an unmap for valid vLPIs

[Marc Zyngier]

Before performing an unmap, let's check that what we have was
really mapped the first place.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 virt/kvm/arm/vgic/vgic-v4.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
index 53c324aa44ef..4a37292855bc 100644
--- a/virt/kvm/arm/vgic/vgic-v4.c
+++ b/virt/kvm/arm/vgic/vgic-v4.c
@@ -337,8 +337,10 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
 		goto out;
 
 	WARN_ON(!(irq->hw && irq->host_irq == virq));
-	irq->hw = false;
-	ret = its_unmap_vlpi(virq);
+	if (irq->hw) {
+		irq->hw = false;
+		ret = its_unmap_vlpi(virq);
+	}
 
 out:
 	mutex_unlock(&its->its_lock);
-- 
2.14.2

[PATCH 6/7] arm64: KVM: fix VTTBR_BADDR_MASK BUG_ON off-by-one

[Marc Zyngier]

From: Kristina Martsenko <kristina.martsenko@arm.com>

VTTBR_BADDR_MASK is used to sanity check the size and alignment of the
VTTBR address. It seems to currently be off by one, thereby only
allowing up to 47-bit addresses (instead of 48-bit) and also
insufficiently checking the alignment. This patch fixes it.

As an example, with 4k pages, before this patch we have:

  PHYS_MASK_SHIFT = 48
  VTTBR_X = 37 - 24 = 13
  VTTBR_BADDR_SHIFT = 13 - 1 = 12
  VTTBR_BADDR_MASK = ((1 << 35) - 1) << 12 = 0x00007ffffffff000

Which is wrong, because the mask doesn't allow bit 47 of the VTTBR
address to be set, and only requires the address to be 12-bit (4k)
aligned, while it actually needs to be 13-bit (8k) aligned because we
concatenate two 4k tables.

With this patch, the mask becomes 0x0000ffffffffe000, which is what we
want.

Fixes: 0369f6a34b9f ("arm64: KVM: EL2 register definitions")
Cc: <stable@vger.kernel.org> # 3.11.x
Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
Signed-off-by: Kristina Martsenko <kristina.martsenko@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm64/include/asm/kvm_arm.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_arm.h b/arch/arm64/include/asm/kvm_arm.h
index 61d694c2eae5..555d463c0eaa 100644
--- a/arch/arm64/include/asm/kvm_arm.h
+++ b/arch/arm64/include/asm/kvm_arm.h
@@ -170,8 +170,7 @@
 #define VTCR_EL2_FLAGS			(VTCR_EL2_COMMON_BITS | VTCR_EL2_TGRAN_FLAGS)
 #define VTTBR_X				(VTTBR_X_TGRAN_MAGIC - VTCR_EL2_T0SZ_IPA)
 
-#define VTTBR_BADDR_SHIFT (VTTBR_X - 1)
-#define VTTBR_BADDR_MASK  (((UL(1) << (PHYS_MASK_SHIFT - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT)
+#define VTTBR_BADDR_MASK  (((UL(1) << (PHYS_MASK_SHIFT - VTTBR_X)) - 1) << VTTBR_X)
 #define VTTBR_VMID_SHIFT  (UL(48))
 #define VTTBR_VMID_MASK(size) (_AT(u64, (1 << size) - 1) << VTTBR_VMID_SHIFT)
 
-- 
2.14.2

[PATCH 7/7] arm: KVM: Fix VTTBR_BADDR_MASK BUG_ON off-by-one

[Marc Zyngier]

VTTBR_BADDR_MASK is used to sanity check the size and alignment of the
VTTBR address. It seems to currently be off by one, thereby only
allowing up to 39-bit addresses (instead of 40-bit) and also
insufficiently checking the alignment. This patch fixes it.

This patch is the 32bit pendent of Kristina's arm64 fix, and
she deserves the actual kudos for pinpointing that one.

Fixes: f7ed45be3ba52 ("KVM: ARM: World-switch implementation")
Cc: <stable@vger.kernel.org> # 3.9
Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
 arch/arm/include/asm/kvm_arm.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
index c8781450905b..3ab8b3781bfe 100644
--- a/arch/arm/include/asm/kvm_arm.h
+++ b/arch/arm/include/asm/kvm_arm.h
@@ -161,8 +161,7 @@
 #else
 #define VTTBR_X		(5 - KVM_T0SZ)
 #endif
-#define VTTBR_BADDR_SHIFT (VTTBR_X - 1)
-#define VTTBR_BADDR_MASK  (((_AC(1, ULL) << (40 - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT)
+#define VTTBR_BADDR_MASK  (((_AC(1, ULL) << (40 - VTTBR_X)) - 1) << VTTBR_X)
 #define VTTBR_VMID_SHIFT  _AC(48, ULL)
 #define VTTBR_VMID_MASK(size)	(_AT(u64, (1 << size) - 1) << VTTBR_VMID_SHIFT)
 
-- 
2.14.2

[PATCH 7/7] arm: KVM: Fix VTTBR_BADDR_MASK BUG_ON off-by-one

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:21PM +0000, Marc Zyngier wrote:
> VTTBR_BADDR_MASK is used to sanity check the size and alignment of the
> VTTBR address. It seems to currently be off by one, thereby only
> allowing up to 39-bit addresses (instead of 40-bit) and also
> insufficiently checking the alignment. This patch fixes it.
> 
> This patch is the 32bit pendent of Kristina's arm64 fix, and
> she deserves the actual kudos for pinpointing that one.
> 
> Fixes: f7ed45be3ba52 ("KVM: ARM: World-switch implementation")
> Cc: <stable@vger.kernel.org> # 3.9
> Reported-by: Kristina Martsenko <kristina.martsenko@arm.com>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
>  arch/arm/include/asm/kvm_arm.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index c8781450905b..3ab8b3781bfe 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -161,8 +161,7 @@
>  #else
>  #define VTTBR_X		(5 - KVM_T0SZ)
>  #endif
> -#define VTTBR_BADDR_SHIFT (VTTBR_X - 1)
> -#define VTTBR_BADDR_MASK  (((_AC(1, ULL) << (40 - VTTBR_X)) - 1) << VTTBR_BADDR_SHIFT)
> +#define VTTBR_BADDR_MASK  (((_AC(1, ULL) << (40 - VTTBR_X)) - 1) << VTTBR_X)
>  #define VTTBR_VMID_SHIFT  _AC(48, ULL)
>  #define VTTBR_VMID_MASK(size)	(_AT(u64, (1 << size) - 1) << VTTBR_VMID_SHIFT)
>  
> -- 
> 2.14.2
> 

[PATCH 1/7] KVM: arm/arm64: vgic-irqfd: Fix MSI entry allocation

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:15PM +0000, Marc Zyngier wrote:
> Using the size of the structure we're allocating is a good idea
> and avoids any surprise... In this case, we're happilly confusing
> kvm_kernel_irq_routing_entry and kvm_irq_routing_entry...

Yikes.

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> 
> Fixes: 95b110ab9a09 ("KVM: arm/arm64: Enable irqchip routing")
> Cc: stable at vger.kernel.org # 4.8
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-irqfd.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-irqfd.c b/virt/kvm/arm/vgic/vgic-irqfd.c
> index b7baf581611a..99e026d2dade 100644
> --- a/virt/kvm/arm/vgic/vgic-irqfd.c
> +++ b/virt/kvm/arm/vgic/vgic-irqfd.c
> @@ -112,8 +112,7 @@ int kvm_vgic_setup_default_irq_routing(struct kvm *kvm)
>  	u32 nr = dist->nr_spis;
>  	int i, ret;
>  
> -	entries = kcalloc(nr, sizeof(struct kvm_kernel_irq_routing_entry),
> -			  GFP_KERNEL);
> +	entries = kcalloc(nr, sizeof(*entries), GFP_KERNEL);
>  	if (!entries)
>  		return -ENOMEM;
>  
> -- 
> 2.14.2
> 

[PATCH 2/7] KVM: arm/arm64: vgic: Preserve the revious read from the pending table

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:16PM +0000, Marc Zyngier wrote:
> The current pending table parsing code assumes that we keep the
> previous read of the pending bits, but keep that variable in
> the current block, making sure it is discarded on each loop.
> 
> We end-up using whatever is on the stack. Who knows, it might
> just be the right thing...

ouch, again.

> 
> Fixes: 280771252c1ba ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
> Cc: stable at vger.kernel.org # 4.12
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v3.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 2f05f732d3fd..f47e8481fa45 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -327,13 +327,13 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  	int last_byte_offset = -1;
>  	struct vgic_irq *irq;
>  	int ret;
> +	u8 val;
>  
>  	list_for_each_entry(irq, &dist->lpi_list_head, lpi_list) {
>  		int byte_offset, bit_nr;
>  		struct kvm_vcpu *vcpu;
>  		gpa_t pendbase, ptr;
>  		bool stored;
> -		u8 val;
>  
>  		vcpu = irq->target_vcpu;
>  		if (!vcpu)
> -- 
> 2.14.2
> 

[PATCH 3/7] KVM: arm/arm64: vgic-its: Preserve the revious read from the pending table

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:17PM +0000, Marc Zyngier wrote:
> The current pending table parsing code assumes that we keep the
> previous read of the pending bits, but keep that variable in
> the current block, making sure it is discarded on each loop.
> 
> We end-up using whatever is on the stack. Who knows, it might
> just be the right thing...

And the hits just keep on coming...

> 
> Fixes: 33d3bc9556a7d ("KVM: arm64: vgic-its: Read initial LPI pending table")
> Cc: stable at vger.kernel.org # 4.8
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-its.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index a3754ec719c4..370086006838 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -421,6 +421,7 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>  	u32 *intids;
>  	int nr_irqs, i;
>  	unsigned long flags;
> +	u8 pendmask;
>  
>  	nr_irqs = vgic_copy_lpi_list(vcpu, &intids);
>  	if (nr_irqs < 0)
> @@ -428,7 +429,6 @@ static int its_sync_lpi_pending_table(struct kvm_vcpu *vcpu)
>  
>  	for (i = 0; i < nr_irqs; i++) {
>  		int byte_offset, bit_nr;
> -		u8 pendmask;
>  
>  		byte_offset = intids[i] / BITS_PER_BYTE;
>  		bit_nr = intids[i] % BITS_PER_BYTE;
> -- 
> 2.14.2
> 

[PATCH 4/7] KVM: arm/arm64: vgic-its: Check result of allocation before use

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:18PM +0000, Marc Zyngier wrote:
> We miss a test against NULL after allocation.
> 
> Fixes: 6d03a68f8054 ("KVM: arm64: vgic-its: Turn device_id validation into generic ID validation")
> Cc: stable at vger.kernel.org # 4.8
> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
>  virt/kvm/arm/vgic/vgic-its.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
> index 370086006838..30f7c7e6d2f4 100644
> --- a/virt/kvm/arm/vgic/vgic-its.c
> +++ b/virt/kvm/arm/vgic/vgic-its.c
> @@ -821,6 +821,8 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>  		return E_ITS_MAPC_COLLECTION_OOR;
>  
>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
> +	if (!collection)
> +		return -ENOMEM;

Our processing of this return value seems to be "shrug, something went
wrong, let's move on to the next command".  Is this really a valid thing
to do if we're so much under memory pressure that we dannot allocate a
collection structure?

My question notwithstanding:

Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

>  
>  	collection->collection_id = coll_id;
>  	collection->target_addr = COLLECTION_NOT_MAPPED;
> -- 
> 2.14.2
> 

[PATCH 5/7] KVM: arm/arm64: vgic-v4: Only perform an unmap for valid vLPIs

[Christoffer Dall]

On Thu, Nov 16, 2017 at 05:58:19PM +0000, Marc Zyngier wrote:
> Before performing an unmap, let's check that what we have was
> really mapped the first place.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Christoffer Dall <christoffer.dall@linaro.org>

> ---
>  virt/kvm/arm/vgic/vgic-v4.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index 53c324aa44ef..4a37292855bc 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -337,8 +337,10 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
>  		goto out;
>  
>  	WARN_ON(!(irq->hw && irq->host_irq == virq));
> -	irq->hw = false;
> -	ret = its_unmap_vlpi(virq);
> +	if (irq->hw) {
> +		irq->hw = false;
> +		ret = its_unmap_vlpi(virq);
> +	}
>  
>  out:
>  	mutex_unlock(&its->its_lock);
> -- 
> 2.14.2
> 

[PATCH 4/7] KVM: arm/arm64: vgic-its: Check result of allocation before use

[Marc Zyngier]

On 20/11/17 14:58, Christoffer Dall wrote:
> On Thu, Nov 16, 2017 at 05:58:18PM +0000, Marc Zyngier wrote:
>> We miss a test against NULL after allocation.
>>
>> Fixes: 6d03a68f8054 ("KVM: arm64: vgic-its: Turn device_id validation into generic ID validation")
>> Cc: stable at vger.kernel.org # 4.8
>> Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
>> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
>> ---
>>  virt/kvm/arm/vgic/vgic-its.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/virt/kvm/arm/vgic/vgic-its.c b/virt/kvm/arm/vgic/vgic-its.c
>> index 370086006838..30f7c7e6d2f4 100644
>> --- a/virt/kvm/arm/vgic/vgic-its.c
>> +++ b/virt/kvm/arm/vgic/vgic-its.c
>> @@ -821,6 +821,8 @@ static int vgic_its_alloc_collection(struct vgic_its *its,
>>  		return E_ITS_MAPC_COLLECTION_OOR;
>>  
>>  	collection = kzalloc(sizeof(*collection), GFP_KERNEL);
>> +	if (!collection)
>> +		return -ENOMEM;
> 
> Our processing of this return value seems to be "shrug, something went
> wrong, let's move on to the next command".  Is this really a valid thing
> to do if we're so much under memory pressure that we dannot allocate a
> collection structure?

The main issue is that we don't currently have a good story when
returning error in general. The current implementation of the vITS
doesn't implement the stalling behaviour which would give the guest the
opportunity to do something. From a host PoV, I'm not really sure what
we can do. Killing the guest doesn't seem very appropriate.

So I'd rather implement stalling the command queue (see 6.3.2 in the
GICv3 spec), and implement something to report the error (even though
this is IMPDEF). Thoughts?

> 
> My question notwithstanding:
> 
> Acked-by: Christoffer Dall <christoffer.dall@linaro.org>

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...