linux-kernel - Re: [PATCH v1 02/11] KVM: x86: add kvm_apic_map_get_dest

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: <20160701123941.GA27840@potion>
Date:	Fri, 1 Jul 2016 14:39:42 +0200
From:	Radim Krčmář <rkrcmar@...hat.com>
To:	Paolo Bonzini <pbonzini@...hat.com>
Cc:	linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
	"Lan, Tianyu" <tianyu.lan@...el.com>,
	Igor Mammedov <imammedo@...hat.com>,
	Jan Kiszka <jan.kiszka@....de>, Peter Xu <peterx@...hat.com>
Subject: Re: [PATCH v1 02/11] KVM: x86: add kvm_apic_map_get_dest_lapic

2016-07-01 09:57+0200, Paolo Bonzini:
> On 30/06/2016 22:54, Radim Krčmář wrote:
>> kvm_irq_delivery_to_apic_fast and kvm_intr_is_single_vcpu_fast both
>> compute the interrupt destination.  Factor the code.
>> 
>> 'struct kvm_lapic **dst = NULL' had to be added to silence GCC.
>> GCC might complain about potential NULL access in the future, because it
>> missed conditions that avoided uninitialized uses of dst.
>> 
>> Signed-off-by: Radim Krčmář <rkrcmar@...hat.com>
>> ---
>>  v1: improved comment for kvm_apic_map_get_dest_lapic() [Peter]
>> 
>>  arch/x86/kvm/lapic.c | 241 ++++++++++++++++++++++-----------------------------
>>  1 file changed, 103 insertions(+), 138 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
>> index 22a6474af220..238b87b068db 100644
>> --- a/arch/x86/kvm/lapic.c
>> +++ b/arch/x86/kvm/lapic.c
>> @@ -671,14 +671,98 @@ static void kvm_apic_disabled_lapic_found(struct kvm *kvm)
>>  	}
>>  }
>>  
>> +/* Return true if the interrupt can be handled by using *bitmap as index mask
>> + * for valid destinations in *dst array.
>> + * Return false if kvm_apic_map_get_dest_lapic did nothing useful.
>> + * Note: we may have zero kvm_lapic destinations when we return true, which
>> + * means that the interrupt should be dropped.  In this case, *bitmap would be
>> + * zero and *dst undefined.
>> + */
>> +static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, struct kvm_lapic *src,
>> +		struct kvm_lapic_irq *irq, struct kvm_apic_map *map,
>> +		struct kvm_lapic ***dst, unsigned long *bitmap)
>> +{
>> +	int i, lowest;
>> +	bool x2apic_ipi;
>> +	u16 cid;
>> +
>> +	if (irq->shorthand == APIC_DEST_SELF) {
>> +		*dst = &src;
> 
> This is not valid, &src dies as soon as you leave the function.  You
> need to pass &src into kvm_apic_map_get_dest_lapic and here do something
> like

Indeed, that would have been bad.

> 	if (irq->shorthand) {
> 		if (irq->shorthand == APIC_DEST_SELF && p_src) {
> 			*dst = p_src;
> 			*bitmap = 1;
> 			return true;
> 		}
> 		return false;
> 	}
> 
> If it's not too hard, I'd like to have patch 4 before this one.

Sure, thanks.