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

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]

Date:   Wed, 29 Aug 2018 17:42:47 +0200
From:   Radim Krcmar <rkrcmar@...hat.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Wanpeng Li <kernellwp@...il.com>,
        Liran Alon <liran.alon@...cle.com>,
        LKML <linux-kernel@...r.kernel.org>, kvm <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH] KVM: LAPIC: Fix pv ipis out-of-bounds access

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@...cle.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))