* [PATCH v2 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context
2022-06-07 13:14 [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
@ 2022-06-07 13:14 ` Marc Zyngier
2022-06-07 13:14 ` [PATCH v2 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending Marc Zyngier
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2022-06-07 13:14 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel, kvm
Cc: Eric Auger, Ricardo Koller, James Morse, Suzuki K Poulose,
Alexandru Elisei, Oliver Upton, kernel-team
Since 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state
from the HW"), we're able to source the pending bit for an interrupt
that is stored either on the physical distributor or on a device.
However, this state is only available when the vcpu is loaded,
and is not intended to be accessed from userspace. Unfortunately,
the GICv2 emulation doesn't provide specific userspace accessors,
and we fallback with the ones that are intended for the guest,
with fatal consequences.
Add a new vgic_uaccess_read_pending() accessor for userspace
to use, build on top of the existing vgic_mmio_read_pending().
Reported-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Tested-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
Fixes: 5bfa685e62e9 ("KVM: arm64: vgic: Read HW interrupt pending state from the HW")
---
arch/arm64/kvm/vgic/vgic-mmio-v2.c | 4 ++--
arch/arm64/kvm/vgic/vgic-mmio.c | 19 ++++++++++++++++---
arch/arm64/kvm/vgic/vgic-mmio.h | 3 +++
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v2.c b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
index 77a67e9d3d14..e070cda86e12 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v2.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v2.c
@@ -429,11 +429,11 @@ static const struct vgic_register_region vgic_v2_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_SET,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- NULL, vgic_uaccess_write_spending, 1,
+ vgic_uaccess_read_pending, vgic_uaccess_write_spending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_PENDING_CLEAR,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
- NULL, vgic_uaccess_write_cpending, 1,
+ vgic_uaccess_read_pending, vgic_uaccess_write_cpending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ(GIC_DIST_ACTIVE_SET,
vgic_mmio_read_active, vgic_mmio_write_sactive,
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index 49837d3a3ef5..dc8c52487e47 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -226,8 +226,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
return 0;
}
-unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
+static unsigned long __read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len,
+ bool is_user)
{
u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
u32 value = 0;
@@ -248,7 +249,7 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
IRQCHIP_STATE_PENDING,
&val);
WARN_RATELIMIT(err, "IRQ %d", irq->host_irq);
- } else if (vgic_irq_is_mapped_level(irq)) {
+ } else if (!is_user && vgic_irq_is_mapped_level(irq)) {
val = vgic_get_phys_line_level(irq);
} else {
val = irq_is_pending(irq);
@@ -263,6 +264,18 @@ unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
return value;
}
+unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ return __read_pending(vcpu, addr, len, false);
+}
+
+unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ return __read_pending(vcpu, addr, len, true);
+}
+
static bool is_vgic_v2_sgi(struct kvm_vcpu *vcpu, struct vgic_irq *irq)
{
return (vgic_irq_is_sgi(irq->intid) &&
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.h b/arch/arm64/kvm/vgic/vgic-mmio.h
index 3fa696f198a3..6082d4b66d39 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.h
+++ b/arch/arm64/kvm/vgic/vgic-mmio.h
@@ -149,6 +149,9 @@ int vgic_uaccess_write_cenable(struct kvm_vcpu *vcpu,
unsigned long vgic_mmio_read_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len);
+unsigned long vgic_uaccess_read_pending(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len);
+
void vgic_mmio_write_spending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val);
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
2022-06-07 13:14 [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
2022-06-07 13:14 ` [PATCH v2 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context Marc Zyngier
@ 2022-06-07 13:14 ` Marc Zyngier
2022-06-07 19:28 ` Eric Auger
2022-06-07 13:14 ` [PATCH v2 3/3] KVM: arm64: Warn if accessing timer pending state outside of vcpu context Marc Zyngier
2022-06-07 15:27 ` (subset) [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
3 siblings, 1 reply; 6+ messages in thread
From: Marc Zyngier @ 2022-06-07 13:14 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel, kvm
Cc: Eric Auger, Ricardo Koller, James Morse, Suzuki K Poulose,
Alexandru Elisei, Oliver Upton, kernel-team
Now that GICv2 has a proper userspace accessor for the pending state,
switch GICv3 over to it, dropping the local version, moving over the
specific behaviours that CGIv3 requires (such as the distinction
between pending latch and line level which were never enforced
with GICv2).
We also gain extra locking that isn't really necessary for userspace,
but that's a small price to pay for getting rid of superfluous code.
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++----------------------------
arch/arm64/kvm/vgic/vgic-mmio.c | 21 +++++++++++++++-
2 files changed, 22 insertions(+), 39 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index f7aa7bcd6fb8..f15e29cc63ce 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
return 0;
}
-static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
- gpa_t addr, unsigned int len)
-{
- u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
- u32 value = 0;
- int i;
-
- /*
- * pending state of interrupt is latched in pending_latch variable.
- * Userspace will save and restore pending state and line_level
- * separately.
- * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
- * for handling of ISPENDR and ICPENDR.
- */
- for (i = 0; i < len * 8; i++) {
- struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
- bool state = irq->pending_latch;
-
- if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
- int err;
-
- err = irq_get_irqchip_state(irq->host_irq,
- IRQCHIP_STATE_PENDING,
- &state);
- WARN_ON(err);
- }
-
- if (state)
- value |= (1U << i);
-
- vgic_put_irq(vcpu->kvm, irq);
- }
-
- return value;
-}
-
static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len,
unsigned long val)
@@ -666,7 +630,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
@@ -750,7 +714,7 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_spending,
- vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
+ vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
vgic_mmio_read_pending, vgic_mmio_write_cpending,
diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
index dc8c52487e47..997d0fce2088 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio.c
@@ -240,6 +240,15 @@ static unsigned long __read_pending(struct kvm_vcpu *vcpu,
unsigned long flags;
bool val;
+ /*
+ * When used from userspace with a GICv3 model:
+ *
+ * Pending state of interrupt is latched in pending_latch
+ * variable. Userspace will save and restore pending state
+ * and line_level separately.
+ * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
+ * for handling of ISPENDR and ICPENDR.
+ */
raw_spin_lock_irqsave(&irq->irq_lock, flags);
if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
int err;
@@ -252,7 +261,17 @@ static unsigned long __read_pending(struct kvm_vcpu *vcpu,
} else if (!is_user && vgic_irq_is_mapped_level(irq)) {
val = vgic_get_phys_line_level(irq);
} else {
- val = irq_is_pending(irq);
+ switch (vcpu->kvm->arch.vgic.vgic_model) {
+ case KVM_DEV_TYPE_ARM_VGIC_V3:
+ if (is_user) {
+ val = irq->pending_latch;
+ break;
+ }
+ fallthrough;
+ default:
+ val = irq_is_pending(irq);
+ break;
+ }
}
value |= ((u32)val << i);
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending
2022-06-07 13:14 ` [PATCH v2 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending Marc Zyngier
@ 2022-06-07 19:28 ` Eric Auger
0 siblings, 0 replies; 6+ messages in thread
From: Eric Auger @ 2022-06-07 19:28 UTC (permalink / raw)
To: Marc Zyngier, kvmarm, linux-arm-kernel, kvm
Cc: Ricardo Koller, James Morse, Suzuki K Poulose, Alexandru Elisei,
Oliver Upton, kernel-team
Hi Marc,
On 6/7/22 15:14, Marc Zyngier wrote:
> Now that GICv2 has a proper userspace accessor for the pending state,
> switch GICv3 over to it, dropping the local version, moving over the
> specific behaviours that CGIv3 requires (such as the distinction
> between pending latch and line level which were never enforced
> with GICv2).
>
> We also gain extra locking that isn't really necessary for userspace,
> but that's a small price to pay for getting rid of superfluous code.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 40 ++----------------------------
> arch/arm64/kvm/vgic/vgic-mmio.c | 21 +++++++++++++++-
> 2 files changed, 22 insertions(+), 39 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index f7aa7bcd6fb8..f15e29cc63ce 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -353,42 +353,6 @@ static unsigned long vgic_mmio_read_v3_idregs(struct kvm_vcpu *vcpu,
> return 0;
> }
>
> -static unsigned long vgic_v3_uaccess_read_pending(struct kvm_vcpu *vcpu,
> - gpa_t addr, unsigned int len)
> -{
> - u32 intid = VGIC_ADDR_TO_INTID(addr, 1);
> - u32 value = 0;
> - int i;
> -
> - /*
> - * pending state of interrupt is latched in pending_latch variable.
> - * Userspace will save and restore pending state and line_level
> - * separately.
> - * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
> - * for handling of ISPENDR and ICPENDR.
> - */
> - for (i = 0; i < len * 8; i++) {
> - struct vgic_irq *irq = vgic_get_irq(vcpu->kvm, vcpu, intid + i);
> - bool state = irq->pending_latch;
> -
> - if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> - int err;
> -
> - err = irq_get_irqchip_state(irq->host_irq,
> - IRQCHIP_STATE_PENDING,
> - &state);
> - WARN_ON(err);
> - }
> -
> - if (state)
> - value |= (1U << i);
> -
> - vgic_put_irq(vcpu->kvm, irq);
> - }
> -
> - return value;
> -}
> -
> static int vgic_v3_uaccess_write_pending(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len,
> unsigned long val)
> @@ -666,7 +630,7 @@ static const struct vgic_register_region vgic_v3_dist_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ISPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 1,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_BITS_PER_IRQ_SHARED(GICD_ICPENDR,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> @@ -750,7 +714,7 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ISPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_spending,
> - vgic_v3_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> + vgic_uaccess_read_pending, vgic_v3_uaccess_write_pending, 4,
> VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH_UACCESS(SZ_64K + GICR_ICPENDR0,
> vgic_mmio_read_pending, vgic_mmio_write_cpending,
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio.c b/arch/arm64/kvm/vgic/vgic-mmio.c
> index dc8c52487e47..997d0fce2088 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio.c
> @@ -240,6 +240,15 @@ static unsigned long __read_pending(struct kvm_vcpu *vcpu,
> unsigned long flags;
> bool val;
>
> + /*
> + * When used from userspace with a GICv3 model:
> + *
> + * Pending state of interrupt is latched in pending_latch
> + * variable. Userspace will save and restore pending state
> + * and line_level separately.
> + * Refer to Documentation/virt/kvm/devices/arm-vgic-v3.rst
> + * for handling of ISPENDR and ICPENDR.
> + */
> raw_spin_lock_irqsave(&irq->irq_lock, flags);
> if (irq->hw && vgic_irq_is_sgi(irq->intid)) {
> int err;
> @@ -252,7 +261,17 @@ static unsigned long __read_pending(struct kvm_vcpu *vcpu,
> } else if (!is_user && vgic_irq_is_mapped_level(irq)) {
> val = vgic_get_phys_line_level(irq);
> } else {
> - val = irq_is_pending(irq);
> + switch (vcpu->kvm->arch.vgic.vgic_model) {
> + case KVM_DEV_TYPE_ARM_VGIC_V3:
> + if (is_user) {
> + val = irq->pending_latch;
> + break;
> + }
> + fallthrough;
> + default:
> + val = irq_is_pending(irq);
> + break;
> + }
> }
>
> value |= ((u32)val << i);
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Thanks!
Eric
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] KVM: arm64: Warn if accessing timer pending state outside of vcpu context
2022-06-07 13:14 [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
2022-06-07 13:14 ` [PATCH v2 1/3] KVM: arm64: Don't read a HW interrupt pending state in user context Marc Zyngier
2022-06-07 13:14 ` [PATCH v2 2/3] KVM: arm64: Replace vgic_v3_uaccess_read_pending with vgic_uaccess_read_pending Marc Zyngier
@ 2022-06-07 13:14 ` Marc Zyngier
2022-06-07 15:27 ` (subset) [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
3 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2022-06-07 13:14 UTC (permalink / raw)
To: kvmarm, linux-arm-kernel, kvm
Cc: Eric Auger, Ricardo Koller, James Morse, Suzuki K Poulose,
Alexandru Elisei, Oliver Upton, kernel-team
A recurrent bug in the KVM/arm64 code base consists in trying to
access the timer pending state outside of the vcpu context, which
makes zero sense (the pending state only exists when the vcpu
is loaded).
In order to avoid more embarassing crashes and catch the offenders
red-handed, add a warning to kvm_arch_timer_get_input_level() and
return the state as non-pending. This avoids taking the system down,
and still helps tracking down silly bugs.
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
arch/arm64/kvm/arch_timer.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/kvm/arch_timer.c b/arch/arm64/kvm/arch_timer.c
index 4e39ace073af..3b8d062e30ea 100644
--- a/arch/arm64/kvm/arch_timer.c
+++ b/arch/arm64/kvm/arch_timer.c
@@ -1230,6 +1230,9 @@ bool kvm_arch_timer_get_input_level(int vintid)
struct kvm_vcpu *vcpu = kvm_get_running_vcpu();
struct arch_timer_context *timer;
+ if (WARN(!vcpu, "No vcpu context!\n"))
+ return false;
+
if (vintid == vcpu_vtimer(vcpu)->irq.irq)
timer = vcpu_vtimer(vcpu);
else if (vintid == vcpu_ptimer(vcpu)->irq.irq)
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: (subset) [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state
2022-06-07 13:14 [PATCH v2 0/3] KVM: arm64: Fix userspace access to HW pending state Marc Zyngier
` (2 preceding siblings ...)
2022-06-07 13:14 ` [PATCH v2 3/3] KVM: arm64: Warn if accessing timer pending state outside of vcpu context Marc Zyngier
@ 2022-06-07 15:27 ` Marc Zyngier
3 siblings, 0 replies; 6+ messages in thread
From: Marc Zyngier @ 2022-06-07 15:27 UTC (permalink / raw)
To: Marc Zyngier, kvm, linux-arm-kernel, kvmarm; +Cc: kernel-team
On Tue, 7 Jun 2022 14:14:24 +0100, Marc Zyngier wrote:
> This is a followup from [1], which aims at fixing userspace access to
> interrupt pending state for HW interrupts.
>
> * From v1 [1]:
> - Keep some of the GICv3-specifics around to avoid regressing the
> line vs latch distinction (Eric).
>
> [...]
Applied to fixes, thanks!
[1/3] KVM: arm64: Don't read a HW interrupt pending state in user context
commit: bfb5ed4097c75917da5c8a74ab236b5acc703d08
Cheers,
M.
--
Marc Zyngier <maz@kernel.org>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 6+ messages in thread