Date:   Wed, 25 Aug 2021 13:41:20 +0300
From:   Maxim Levitsky <mlevitsk@...hat.com>
To:     Vitaly Kuznetsov <vkuznets@...hat.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        "Dr. David Alan Gilbert" <dgilbert@...hat.com>,
        Nitesh Narayan Lal <nitesh@...hat.com>,
        linux-kernel@...r.kernel.org, Eduardo Habkost <ehabkost@...hat.com>
Subject: Re: [PATCH v2 4/4] KVM: x86: Fix stack-out-of-bounds memory access
 from ioapic_write_indirect()

On Wed, 2021-08-25 at 11:43 +0200, Vitaly Kuznetsov wrote:
> Maxim Levitsky <mlevitsk@...hat.com> writes:
> 
> > On Wed, 2021-08-25 at 10:21 +0200, Vitaly Kuznetsov wrote:
> > > Maxim Levitsky <mlevitsk@...hat.com> writes:
> > > 
> > > > On Tue, 2021-08-24 at 16:42 +0200, Vitaly Kuznetsov wrote:
> > > ...
> > > > Not a classical review but,
> > > > I did some digital archaeology with this one, trying to understand what is going on:
> > > > 
> > > > 
> > > > I think that 16 bit vcpu bitmap is due to the fact that IOAPIC spec states that
> > > > it can address up to 16 cpus in physical destination mode.
> > > >  
> > > > In logical destination mode, assuming flat addressing and that logical id = 1 << physical id
> > > > which KVM hardcodes, it is also only possible to address 8 CPUs.
> > > >  
> > > > However(!) in flat cluster mode, the logical apic id is split in two.
> > > > We have 16 clusters and each have 4 CPUs, so it is possible to address 64 CPUs,
> > > > and unlike the logical ID, the KVM does honour cluster ID, 
> > > > thus one can stick say cluster ID 0 to any vCPU.
> > > >  
> > > >  
> > > > Let's look at ioapic_write_indirect.
> > > > It does:
> > > >  
> > > >     -> bitmap_zero(&vcpu_bitmap, 16);
> > > >     -> kvm_bitmap_or_dest_vcpus(ioapic->kvm, &irq, &vcpu_bitmap);
> > > >     -> kvm_make_scan_ioapic_request_mask(ioapic->kvm, &vcpu_bitmap); // use of the above bitmap
> > > >  
> > > >  
> > > > When we call kvm_bitmap_or_dest_vcpus, we can already overflow the bitmap,
> > > > since we pass all 8 bit of the destination even when it is physical.
> > > >  
> > > >  
> > > > Lets examine the kvm_bitmap_or_dest_vcpus:
> > > >  
> > > >   -> It calls the kvm_apic_map_get_dest_lapic which 
> > > >  
> > > >        -> for physical destinations, it just sets the bitmap, which can overflow
> > > >           if we pass it 8 bit destination (which basically includes reserved bits + 4 bit destination).
> > > >  
> > > >  
> > > >        -> For logical apic ID, it seems to truncate the result to 16 bit, which isn't correct as I explained
> > > >           above, but should not overflow the result.
> > > >  
> > > >   
> > > >    -> If call to kvm_apic_map_get_dest_lapic fails, it goes over all vcpus and tries to match the destination
> > > >        This can overflow as well.
> > > >  
> > > >  
> > > > I also don't like that ioapic_write_indirect calls the kvm_bitmap_or_dest_vcpus twice,
> > > > and second time with 'old_dest_id'
> > > >  
> > > > I am not 100%  sure why old_dest_id/old_dest_mode are needed as I don't see anything in the
> > > > function changing them.
> > > > I think only the guest can change them, so maybe the code deals with the guest changing them
> > > > while the code is running from a different vcpu?
> > > >  
> > > > The commit that introduced this code is 7ee30bc132c683d06a6d9e360e39e483e3990708
> > > > Nitesh Narayan Lal, maybe you remember something about it?
> > > >  
> > > 
> > > Before posting this patch I've contacted Nitesh privately, he's
> > > currently on vacation but will take a look when he gets back.
> > > 
> > > > Also I worry a lot about other callers of kvm_apic_map_get_dest_lapic
> > > >  
> > > > It is also called from kvm_irq_delivery_to_apic_fast, and from kvm_intr_is_single_vcpu_fast
> > > > and both seem to also use 'unsigned long' for bitmap, and then only use 16 bits of it.
> > > >  
> > > > I haven't dug into them, but these don't seem to be IOAPIC related and I think
> > > > can overwrite the stack as well.
> > > 
> > > I'm no expert in this code but when writing the patch I somehow
> > > convinced myself that a single unsigned long is always enough. I think
> > > that for cluster mode 'bitmap' needs 64-bits (and it is *not* a
> > > vcpu_bitmap, we need to convert). I may be completely wrong of course
> > > but in any case this is a different issue. In ioapic_write_indirect() we
> > > have 'vcpu_bitmap' which should certainly be longer than 64 bits.
> > 
> > This code which I mentioned in 'other callers' as far as I see is not IOAPIC related.
> > For regular local APIC all bets are off, any vCPU and apic ID are possible 
> > (xapic I think limits apic id to 255 but x2apic doesn't).
> > 
> > I strongly suspect that this code can overflow as well.
> 
> I've probably missed something but I don't see how
> kvm_apic_map_get_dest_lapic() can set bits above 64 in 'bitmap'. If it
> can, then we have a problem indeed.

It me that missed that '*bitmap = 1' is harmless, since this function returns
the destanation local apic, and bitmap of vCPU to work on starting from
this local apic. So its OK.

So now your patch looks OK to me now.

You can also zero uppper 4 bits of destanation
apic ID though since IO apic spec says that they are reserved.
(But I won't be surprised if that restriction was later lifted, as I am reading
the original IO apic spec, about the good old actual chip).
But that isn't a serious issue.

Reviewed-by: Maxim Levitsky <mlevitsk@...hat.com>


Best regards,
	Maxim Levitsky





> 

Powered by blists - more mailing lists