Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access

[Radim Krcmar <rkrcmar@redhat.com>]

2018-08-29 13:29+0300, Dan Carpenter:
> On Wed, Aug 29, 2018 at 06:23:08PM +0800, Wanpeng Li wrote:
> > On Wed, 29 Aug 2018 at 18:18, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > >
> > > On Wed, Aug 29, 2018 at 01:12:05PM +0300, Dan Carpenter wrote:
> > > > On Wed, Aug 29, 2018 at 12:05:06PM +0300, Liran Alon wrote:
> > > > > > arch/x86/kvm/lapic.c | 17 +++++++++++++----
> > > > > > 1 file changed, 13 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
> > > > > > index 0cefba2..86e933c 100644
> > > > > > --- a/arch/x86/kvm/lapic.c
> > > > > > +++ b/arch/x86/kvm/lapic.c
> > > > > > @@ -571,18 +571,27 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low,
> > > > > >   rcu_read_lock();
> > > > > >   map = rcu_dereference(kvm->arch.apic_map);
> > > > > >
> > > > > > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_low)) < min))
> > > > > > +         goto out;
> > > > >
> > > > > I personally think “if ((min + __fls(ipi_bitmap_low)) > map->max_apic_id)” is more readable.
> > > > > But that’s just a matter of taste :)
> > > >
> > > > That's an integer overflow.
> > > >
> > > > But I do prefer to put the variable on the left.  The truth is that some
> > > > Smatch checks just ignore code which is backwards written because
> > > > otherwise you have to write duplicate code and the most code is written
> > > > with the variable on the left.
> > > >
> > > >       if (min > (s32)(map->max_apic_id - __fls(ipi_bitmap_low))
> > >
> > > Wait, the (s32) cast doesn't make sense.  We want negative min values to
> > > be treated as invalid.
> > 
> > In v2, how about:
> > 
> > if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) >
> > map->max_apic_id))
> >     goto out;
> 
> That works, too.  It still has the off by one and we should set
> "count = -KVM_EINVAL;".

I'd prefer to ignore destinations that are not present and deliver the
rest, possibly nothing, instead of returning an error.
(It's closer to how the real hardware behaves and we already return the
 number of notified VCPUs, so the caller can tell whether something went
 wrong.)

Either in the form that I have posted earlier, or as:

	if (min > map->max_apic_id)
		goto out;

	for_each_set_bit(i, &ipi_bitmap_low, MIN(BITS_PER_LONG, map->max_apic_id - min + 1))