linux-kernel - Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

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: <86mudjykfa.wl-maz@kernel.org>
Date:   Tue, 29 Oct 2019 09:23:37 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Zenghui Yu <yuzenghui@...wei.com>
Cc:     <eric.auger@...hat.com>, <james.morse@....com>,
        <julien.thierry.kdev@...il.com>, <suzuki.poulose@....com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <kvmarm@...ts.cs.columbia.edu>, <linux-kernel@...r.kernel.org>,
        <wanghaibin.wang@...wei.com>
Subject: Re: [PATCH 3/3] KVM: arm/arm64: vgic: Don't rely on the wrong pending table

On Tue, 29 Oct 2019 07:19:19 +0000,
Zenghui Yu <yuzenghui@...wei.com> wrote:
> 
> It's possible that two LPIs locate in the same "byte_offset" but target
> two different vcpus, where their pending status are indicated by two
> different pending tables.  In such a scenario, using last_byte_offset
> optimization will lead KVM relying on the wrong pending table entry.
> Let us use last_ptr instead, which can be treated as a byte index into
> a pending table and also, can be vcpu specific.
> 
> Signed-off-by: Zenghui Yu <yuzenghui@...wei.com>
> ---
> 
> If this patch has done the right thing, we can even add the:
> 
> Fixes: 280771252c1b ("KVM: arm64: vgic-v3: KVM_DEV_ARM_VGIC_SAVE_PENDING_TABLES")
> 
> But to be honest, I'm not clear about what has this patch actually fixed.
> Pending tables should contain all zeros before we flush vgic_irq's pending
> status into guest's RAM (thinking that guest should never write anything
> into it). So the pending table entry we've read from the guest memory
> seems always be zero. And we will always do the right thing even if we
> rely on the wrong pending table entry.
> 
> I think I must have some misunderstanding here... Please fix me.

I think you're spot on, and it is the code needs fixing, not you! The
problem is that we only read a byte once, irrespective of the vcpu the
interrupts is routed to. If we switch to another vcpu for the same
byte offset, we must reload it.

This can be done by either checking the vcpu, or by tracking the guest
address that we read from (just like you do here).

A small comment below:

>  virt/kvm/arm/vgic/vgic-v3.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 5ef93e5041e1..7cd2e2f81513 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -363,8 +363,8 @@ int vgic_v3_lpi_sync_pending_status(struct kvm *kvm, struct vgic_irq *irq)
>  int vgic_v3_save_pending_tables(struct kvm *kvm)
>  {
>  	struct vgic_dist *dist = &kvm->arch.vgic;
> -	int last_byte_offset = -1;
>  	struct vgic_irq *irq;
> +	gpa_t last_ptr = -1;

This should be written as

     gpa_t last_ptr = ~(gpa_t)0;

>  	int ret;
>  	u8 val;
>  
> @@ -384,11 +384,11 @@ int vgic_v3_save_pending_tables(struct kvm *kvm)
>  		bit_nr = irq->intid % BITS_PER_BYTE;
>  		ptr = pendbase + byte_offset;
>  
> -		if (byte_offset != last_byte_offset) {
> +		if (ptr != last_ptr) {
>  			ret = kvm_read_guest_lock(kvm, ptr, &val, 1);
>  			if (ret)
>  				return ret;
> -			last_byte_offset = byte_offset;
> +			last_ptr = ptr;
>  		}
>  
>  		stored = val & (1U << bit_nr);

Otherwise, this looks good to me (no need to respin for the above
nit).

Eric, can I get an Ack from you, since you write this code?

Thanks,

	M.

-- 
Jazz is not dead, it just smells funny.