| Message ID | 1535521943-5547-1-git-send-email-wanpengli@tencent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: LAPIC: Fix pv ipis out-of-bounds access | expand |
> On 29 Aug 2018, at 8:52, Wanpeng Li <kernellwp@gmail.com> wrote: > > From: Wanpeng Li <wanpengli@tencent.com> > > Dan Carpenter reported that the untrusted data returns from kvm_register_read() > results in the following static checker warning: > arch/x86/kvm/lapic.c:576 kvm_pv_send_ipi() > error: buffer underflow 'map->phys_map' 's32min-s32max' > > KVM guest can easily trigger this by executing the following assembly sequence > in Ring0: > > mov $10, %rax > mov $0xFFFFFFFF, %rbx > mov $0xFFFFFFFF, %rdx > mov $0, %rsi > vmcall > > As this will cause KVM to execute the following code-path: > vmx_handle_exit() -> handle_vmcall() -> kvm_emulate_hypercall() -> kvm_pv_send_ipi() > which will reach out-of-bounds access. > > This patch fixes it by adding a check to kvm_pv_send_ipi() against map->max_apic_id > and also checking whether or not map->phys_map[min + i] is NULL since the max_apic_id > is set according to the max apic id, however, some phys_map maybe NULL when apic id > is sparse, in addition, kvm also unconditionally set max_apic_id to 255 to reserve > enough space for any xAPIC ID. > > Reported-by: Dan Carpenter <dan.carpenter@oracle.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Radim Krčmář <rkrcmar@redhat.com> > Cc: Liran Alon <liran.alon@oracle.com> > Cc: Dan Carpenter <dan.carpenter@oracle.com> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com> > --- > 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 :) > /* Bits above cluster_size are masked in the caller. */ > for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) { > - vcpu = map->phys_map[min + i]->vcpu; > - count += kvm_apic_set_irq(vcpu, &irq, NULL); > + if (map->phys_map[min + i]) { > + vcpu = map->phys_map[min + i]->vcpu; > + count += kvm_apic_set_irq(vcpu, &irq, NULL); > + } > } > > min += cluster_size; > + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min)) > + goto out; > for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) { > - vcpu = map->phys_map[min + i]->vcpu; > - count += kvm_apic_set_irq(vcpu, &irq, NULL); > + if (map->phys_map[min + i]) { > + vcpu = map->phys_map[min + i]->vcpu; > + count += kvm_apic_set_irq(vcpu, &irq, NULL); > + } > } > > +out: > rcu_read_unlock(); > return count; > } > -- > 2.7.4 > Reviewed-By: Liran Alon <liran.alon@oracle.com>
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)) Shouldn't this be >= instead? It looks off by one. regards, dan carpenter
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. regards, dan carpenter
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; 0xFFFFFFFF is converted to int min = -1; Regards, Wanpeng Li
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;". Is the unlikely() really required? I don't know what the fast paths are in KVM, so I don't know. regards, dan carpenter
On Wed, 29 Aug 2018 at 18:29, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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 Sorry, why off by one? > "count = -KVM_EINVAL;". > > Is the unlikely() really required? I don't know what the fast paths are > in KVM, so I don't know. > > regards, > dan carpenter >
> On 29 Aug 2018, at 13:29, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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;". > > Is the unlikely() really required? I don't know what the fast paths are > in KVM, so I don't know. > > regards, > dan carpenter Why is “min” defined as “int” instead of “unsigned int”? It represents the lowest APIC ID in bitmap so it can’t be negative… "if (unlikely(min > map->max_apic_id || (min + __fls(ipi_bitmap_low)) > map->max_apic_id))” should indeed be ok. -Liran
On Wed, Aug 29, 2018 at 06:42:42PM +0800, Wanpeng Li wrote: > On Wed, 29 Aug 2018 at 18:29, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > > > 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 > > Sorry, why off by one? Sorry, my bad. I looked at the code and > is correct. (At first, I thought it should be >= but I hadn't looked at the context). regards, dan carpenter
2018-08-29 13:43+0300, Liran Alon: > Why is “min” defined as “int” instead of “unsigned int”? > It represents the lowest APIC ID in bitmap so it can’t be negative… Right, I think the code would look better as something like (untested): diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 0cefba28c864..24fc84eb97d2 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -548,7 +548,7 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq, } int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, - unsigned long ipi_bitmap_high, int min, + unsigned long ipi_bitmap_high, u32 min, unsigned long icr, int op_64_bit) { int i; @@ -557,6 +557,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, struct kvm_lapic_irq irq = {0}; int cluster_size = op_64_bit ? 64 : 32; int count = 0; + unsigned long ipi_bitmap[2] = {ipi_bitmap_low, ipi_bitmap_high}; irq.vector = icr & APIC_VECTOR_MASK; irq.delivery_mode = icr & APIC_MODE_MASK; @@ -571,16 +572,14 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, rcu_read_lock(); map = rcu_dereference(kvm->arch.apic_map); - /* Bits above cluster_size are masked in the caller. */ - for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) { - vcpu = map->phys_map[min + i]->vcpu; - count += kvm_apic_set_irq(vcpu, &irq, NULL); - } + if (min <= map->max_apic_id) { + size_t ipi_bitmap_size = MIN(sizeof(ipi_bitmap) * 8, + map->max_apic_id - min + 1); - min += cluster_size; - for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) { - vcpu = map->phys_map[min + i]->vcpu; - count += kvm_apic_set_irq(vcpu, &irq, NULL); + for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) { + vcpu = map->phys_map[min + i]->vcpu; + count += kvm_apic_set_irq(vcpu, &irq, NULL); + } } rcu_read_unlock();
2018-08-29 15:55+0200, Radim Krcmar: > 2018-08-29 13:43+0300, Liran Alon: > > Why is “min” defined as “int” instead of “unsigned int”? > > It represents the lowest APIC ID in bitmap so it can’t be negative… > > Right, > > I think the code would look better as something like (untested): > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 0cefba28c864..24fc84eb97d2 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -548,7 +548,7 @@ int kvm_apic_set_irq(struct kvm_vcpu *vcpu, struct kvm_lapic_irq *irq, > } > > int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, > - unsigned long ipi_bitmap_high, int min, > + unsigned long ipi_bitmap_high, u32 min, > unsigned long icr, int op_64_bit) > { > int i; > @@ -557,6 +557,7 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, > struct kvm_lapic_irq irq = {0}; > int cluster_size = op_64_bit ? 64 : 32; > int count = 0; > + unsigned long ipi_bitmap[2] = {ipi_bitmap_low, ipi_bitmap_high}; The patch is wrong, I missed the 32/64 bit cluster size. It's salvageable with something like if (op_64_bit) { ipi_bitmap[0] = ipi_bitmap_low; ipi_bitmap[1] = ipi_bitmap_high; ipi_bitmap_size = 128; } else { ipi_bitmap[0] = (u32)ipi_bitmap_low | ipi_bitmap_high << 32; ipi_bitmap_size = 64; } > > irq.vector = icr & APIC_VECTOR_MASK; > irq.delivery_mode = icr & APIC_MODE_MASK; > @@ -571,16 +572,14 @@ int kvm_pv_send_ipi(struct kvm *kvm, unsigned long ipi_bitmap_low, > rcu_read_lock(); > map = rcu_dereference(kvm->arch.apic_map); > > - /* Bits above cluster_size are masked in the caller. */ > - for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) { > - vcpu = map->phys_map[min + i]->vcpu; > - count += kvm_apic_set_irq(vcpu, &irq, NULL); > - } > + if (min <= map->max_apic_id) { > + size_t ipi_bitmap_size = MIN(sizeof(ipi_bitmap) * 8, > + map->max_apic_id - min + 1); > - min += cluster_size; > - for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) { > - vcpu = map->phys_map[min + i]->vcpu; > - count += kvm_apic_set_irq(vcpu, &irq, NULL); > + for_each_set_bit(i, ipi_bitmap, ipi_bitmap_size) { and ... MIN(ipi_bitmap_size, map->max_apic_id - min + 1) ... Not good, but could still be nicer than the alternatives. > + vcpu = map->phys_map[min + i]->vcpu; > + count += kvm_apic_set_irq(vcpu, &irq, NULL); > + } > } > > rcu_read_unlock();
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))
On Wed, 29 Aug 2018 at 23:42, Radim Krcmar <rkrcmar@redhat.com> wrote: > > 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)) Do it in v2. Regards, Wanpeng Li
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; /* Bits above cluster_size are masked in the caller. */ for_each_set_bit(i, &ipi_bitmap_low, BITS_PER_LONG) { - vcpu = map->phys_map[min + i]->vcpu; - count += kvm_apic_set_irq(vcpu, &irq, NULL); + if (map->phys_map[min + i]) { + vcpu = map->phys_map[min + i]->vcpu; + count += kvm_apic_set_irq(vcpu, &irq, NULL); + } } min += cluster_size; + if (unlikely((s32)(map->max_apic_id - __fls(ipi_bitmap_high)) < min)) + goto out; for_each_set_bit(i, &ipi_bitmap_high, BITS_PER_LONG) { - vcpu = map->phys_map[min + i]->vcpu; - count += kvm_apic_set_irq(vcpu, &irq, NULL); + if (map->phys_map[min + i]) { + vcpu = map->phys_map[min + i]->vcpu; + count += kvm_apic_set_irq(vcpu, &irq, NULL); + } } +out: rcu_read_unlock(); return count; }