linux-kernel - Re: [PATCH v2 4/4] KVM: LAPIC: Don't silently accept bad vectors

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171003175341.GD21107@flask>
Date:   Tue, 3 Oct 2017 19:53:41 +0200
From:   Radim Krčmář <rkrcmar@...hat.com>
To:     Wanpeng Li <kernellwp@...il.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpeng.li@...mail.com>
Subject: Re: [PATCH v2 4/4] KVM: LAPIC: Don't silently accept bad vectors

2017-09-28 18:04-0700, Wanpeng Li:
> From: Wanpeng Li <wanpeng.li@...mail.com>
> 
> Vectors 0-15 are reserved, and a physical LAPIC - upon sending or
> receiving one - would generate an APIC error instead of doing the
> requested action. Make our emulation behave similarly.
> 
> Cc: Paolo Bonzini <pbonzini@...hat.com>
> Cc: Radim Krčmář <rkrcmar@...hat.com>
> Signed-off-by: Wanpeng Li <wanpeng.li@...mail.com>
> ---
>  arch/x86/kvm/lapic.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> index 6bafd06..a779ba9 100644
> --- a/arch/x86/kvm/lapic.c
> +++ b/arch/x86/kvm/lapic.c
> @@ -935,6 +935,25 @@ bool kvm_intr_is_single_vcpu_fast(struct kvm *kvm, struct kvm_lapic_irq *irq,
>  	return ret;
>  }
>  
> +static void apic_error(struct kvm_lapic *apic, unsigned long errmask)
> +{
> +	uint32_t esr;
> +
> +	esr = kvm_lapic_get_reg(apic, APIC_ESR);
> +
> +	if ((esr & errmask) != errmask) {

The spec makes me think that there is going to be only 1 interrupt
(regardless of the number errors) until the software writes 0 to
APIC_ESR.  Is there a better description than the following 10.5.3?

  The ESR is a write/read register. Before attempt to read from the ESR,
  software should first write to it. (The value written does not affect
  the values read subsequently; only zero may be written in x2APIC
  mode.) This write clears any previously logged errors and updates the
  ESR with any errors detected since the last write to the ESR. This
  write also rearms the APIC error interrupt triggering mechanism.

This also describes a different handling of APIC_ESR -- APIC_ESR is
updated only on software writes to APIC_ESR.  All errors in between seem
to be logged internally (not sure where to migrate it).

> +		uint32_t lvterr = kvm_lapic_get_reg(apic, APIC_LVTERR);
> +
> +		kvm_lapic_set_reg(apic, APIC_ESR, esr | errmask);
> +		if (!(lvterr & APIC_LVT_MASKED)) {
> +			struct kvm_lapic_irq irq;
> +
> +			irq.vector = lvterr & 0xff;
> +			kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
> +		}
> +	}
> +}
> +
>  /*
>   * Add a pending IRQ into lapic.
>   * Return 1 if successfully added and 0 if discarded.
> @@ -946,6 +965,11 @@ static int __apic_accept_irq(struct kvm_lapic *apic, int delivery_mode,
>  	int result = 0;
>  	struct kvm_vcpu *vcpu = apic->vcpu;
>  
> +	if (unlikely(vector < 16) && delivery_mode == APIC_DM_FIXED) {
> +		apic_error(apic, APIC_ESR_RECVILL);

The error is also triggered if lowest priority is supported and tries to
deliver an invalid vector.

> +		return 0;
> +	}
> +
>  	trace_kvm_apic_accept_irq(vcpu->vcpu_id, delivery_mode,
>  				  trig_mode, vector);
>  	switch (delivery_mode) {
> @@ -1146,7 +1170,10 @@ static void apic_send_ipi(struct kvm_lapic *apic)
>  		   irq.trig_mode, irq.level, irq.dest_mode, irq.delivery_mode,
>  		   irq.vector, irq.msi_redir_hint);
>  
> -	kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
> +	if (unlikely(irq.vector < 16 && irq.delivery_mode == APIC_DM_FIXED))

Please check how APICv self-IPI acceleration behaves, so we're
consistent.

Thanks.

> +		apic_error(apic, APIC_ESR_SENDILL);
> +	else
> +		kvm_irq_delivery_to_apic(apic->vcpu->kvm, apic, &irq, NULL);
>  }
>  
>  static u32 apic_get_tmcct(struct kvm_lapic *apic)
> @@ -1734,7 +1761,6 @@ int kvm_lapic_reg_write(struct kvm_lapic *apic, u32 reg, u32 val)
>  	case APIC_LVTPC:
>  	case APIC_LVT1:
>  	case APIC_LVTERR:
> -		/* TODO: Check vector */
>  		if (!kvm_apic_sw_enabled(apic))
>  			val |= APIC_LVT_MASKED;
>  
> -- 
> 2.7.4
>