linux-kernel - Re: [PATCH] KVM: arm64: vgic-v3: Drop the reporting of GICR

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4c9c10f9-fc6d-2725-168e-db6442b74574@redhat.com>
Date:   Tue, 17 Nov 2020 18:29:50 +0100
From:   Auger Eric <eric.auger@...hat.com>
To:     Zenghui Yu <yuzenghui@...wei.com>, kvmarm@...ts.cs.columbia.edu,
        maz@...nel.org
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        james.morse@....com, julien.thierry.kdev@...il.com,
        suzuki.poulose@....com, wanghaibin.wang@...wei.com,
        Keqian Zhu <zhukeqian1@...wei.com>
Subject: Re: [PATCH] KVM: arm64: vgic-v3: Drop the reporting of
 GICR_TYPER.Last for userspace

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/c20865a267e44d1e2c0d52ce4e012263@kernel.org/
> 
> Fixes: ba7b3f1275fd ("KVM: arm/arm64: Revisit Redistributor TYPER last bit computation")
> Reported-by: Keqian Zhu <zhukeqian1@...wei.com>
> Signed-off-by: Zenghui Yu <yuzenghui@...wei.com>
Given the state of last bit, it looks sensible atm.

Reviewed-by: Eric Auger <eric.auger@...hat.com>

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,
>