It was recently reported that if GICR_TYPER is accessed before the RD base
address is set, we'll suffer from the unset @rdreg dereferencing. Oops...
gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
(rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
It's "expected" that users will access registers in the redistributor if
the RD has been properly configured (e.g., the RD base address is set). But
it hasn't yet been covered by the existing documentation.
Per discussion on the list [1], the reporting of the GICR_TYPER.Last bit
for userspace never actually worked. And it's difficult for us to emulate
it correctly given that userspace has the flexibility to access it any
time. Let's just drop the reporting of the Last bit for userspace for now
(userspace should have full knowledge about it anyway) and it at least
prevents kernel from panic ;-)
[1] https://lore.kernel.org/kvmarm/[email protected]/
Fixes: ba7b3f1275fd ("KVM: arm/arm64: Revisit Redistributor TYPER last bit computation")
Reported-by: Keqian Zhu <[email protected]>
Signed-off-by: Zenghui Yu <[email protected]>
---
This may be the easiest way to fix the issue and to get the fix backported
to stable tree. There is still some work can be done since (at least) we
have code duplicates between the MMIO and uaccess callbacks.
arch/arm64/kvm/vgic/vgic-mmio-v3.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
index 52d6f24f65dc..15a6c98ee92f 100644
--- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
+++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
@@ -273,6 +273,23 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
return extract_bytes(value, addr & 7, len);
}
+static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
+ gpa_t addr, unsigned int len)
+{
+ unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
+ int target_vcpu_id = vcpu->vcpu_id;
+ u64 value;
+
+ value = (u64)(mpidr & GENMASK(23, 0)) << 32;
+ value |= ((target_vcpu_id & 0xffff) << 8);
+
+ if (vgic_has_its(vcpu->kvm))
+ value |= GICR_TYPER_PLPIS;
+
+ /* reporting of the Last bit is not supported for userspace */
+ return extract_bytes(value, addr & 7, len);
+}
+
static unsigned long vgic_mmio_read_v3r_iidr(struct kvm_vcpu *vcpu,
gpa_t addr, unsigned int len)
{
@@ -593,8 +610,9 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
VGIC_ACCESS_32bit),
- REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
- vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
+ REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_TYPER,
+ vgic_mmio_read_v3r_typer, vgic_mmio_write_wi,
+ vgic_uaccess_read_v3r_typer, vgic_mmio_uaccess_write_wi, 8,
VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
--
2.19.1
Hi Zenghui,
On 11/17/20 4:16 PM, Zenghui Yu wrote:
> It was recently reported that if GICR_TYPER is accessed before the RD base
> address is set, we'll suffer from the unset @rdreg dereferencing. Oops...
>
> gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
>
> It's "expected" that users will access registers in the redistributor if
> the RD has been properly configured (e.g., the RD base address is set). But
> it hasn't yet been covered by the existing documentation.
>
> Per discussion on the list [1], the reporting of the GICR_TYPER.Last bit
> for userspace never actually worked. And it's difficult for us to emulate
> it correctly given that userspace has the flexibility to access it any
> time. Let's just drop the reporting of the Last bit for userspace for now
> (userspace should have full knowledge about it anyway) and it at least
> prevents kernel from panic ;-)
>
> [1] https://lore.kernel.org/kvmarm/[email protected]/
>
> Fixes: ba7b3f1275fd ("KVM: arm/arm64: Revisit Redistributor TYPER last bit computation")
> Reported-by: Keqian Zhu <[email protected]>
> Signed-off-by: Zenghui Yu <[email protected]>
Given the state of last bit, it looks sensible atm.
Reviewed-by: Eric Auger <[email protected]>
Thanks
Eric
> ---
>
> This may be the easiest way to fix the issue and to get the fix backported
> to stable tree. There is still some work can be done since (at least) we
> have code duplicates between the MMIO and uaccess callbacks.
>
> arch/arm64/kvm/vgic/vgic-mmio-v3.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/kvm/vgic/vgic-mmio-v3.c b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> index 52d6f24f65dc..15a6c98ee92f 100644
> --- a/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> +++ b/arch/arm64/kvm/vgic/vgic-mmio-v3.c
> @@ -273,6 +273,23 @@ static unsigned long vgic_mmio_read_v3r_typer(struct kvm_vcpu *vcpu,
> return extract_bytes(value, addr & 7, len);
> }
>
> +static unsigned long vgic_uaccess_read_v3r_typer(struct kvm_vcpu *vcpu,
> + gpa_t addr, unsigned int len)
> +{
> + unsigned long mpidr = kvm_vcpu_get_mpidr_aff(vcpu);
> + int target_vcpu_id = vcpu->vcpu_id;
> + u64 value;
> +
> + value = (u64)(mpidr & GENMASK(23, 0)) << 32;
> + value |= ((target_vcpu_id & 0xffff) << 8);
> +
> + if (vgic_has_its(vcpu->kvm))
> + value |= GICR_TYPER_PLPIS;
> +
> + /* reporting of the Last bit is not supported for userspace */
> + return extract_bytes(value, addr & 7, len);
> +}
> +
> static unsigned long vgic_mmio_read_v3r_iidr(struct kvm_vcpu *vcpu,
> gpa_t addr, unsigned int len)
> {
> @@ -593,8 +610,9 @@ static const struct vgic_register_region vgic_v3_rd_registers[] = {
> REGISTER_DESC_WITH_LENGTH(GICR_IIDR,
> vgic_mmio_read_v3r_iidr, vgic_mmio_write_wi, 4,
> VGIC_ACCESS_32bit),
> - REGISTER_DESC_WITH_LENGTH(GICR_TYPER,
> - vgic_mmio_read_v3r_typer, vgic_mmio_write_wi, 8,
> + REGISTER_DESC_WITH_LENGTH_UACCESS(GICR_TYPER,
> + vgic_mmio_read_v3r_typer, vgic_mmio_write_wi,
> + vgic_uaccess_read_v3r_typer, vgic_mmio_uaccess_write_wi, 8,
> VGIC_ACCESS_64bit | VGIC_ACCESS_32bit),
> REGISTER_DESC_WITH_LENGTH(GICR_WAKER,
> vgic_mmio_read_raz, vgic_mmio_write_wi, 4,
>
On Tue, 17 Nov 2020 23:16:29 +0800, Zenghui Yu wrote:
> It was recently reported that if GICR_TYPER is accessed before the RD base
> address is set, we'll suffer from the unset @rdreg dereferencing. Oops...
>
> gpa_t last_rdist_typer = rdreg->base + GICR_TYPER +
> (rdreg->free_index - 1) * KVM_VGIC_V3_REDIST_SIZE;
>
> It's "expected" that users will access registers in the redistributor if
> the RD has been properly configured (e.g., the RD base address is set). But
> it hasn't yet been covered by the existing documentation.
>
> [...]
Applied to kvm-arm64/fixes-5.10, thanks!
[1/1] KVM: arm64: vgic-v3: Drop the reporting of GICR_TYPER.Last for userspace
commit: 23bde34771f1ea92fb5e6682c0d8c04304d34b3b
I have added a Cc stable, as this definitely wants to be backported.
Cheers,
M.
--
Without deviation from the norm, progress is not possible.