diff mbox series

[v2] ioapic: use irq number instead of vector in ioapic_eoi_broadcast

Message ID 20190622002119.126834-1-liq3ea@163.com (mailing list archive)
State New, archived
Headers show
Series [v2] ioapic: use irq number instead of vector in ioapic_eoi_broadcast | expand

Commit Message

Li Qiang June 22, 2019, 12:21 a.m. UTC 
When emulating irqchip in qemu, such as following command:

x86_64-softmmu/qemu-system-x86_64 -m 1024 -smp 4 -hda /home/test/test.img
-machine kernel-irqchip=off --enable-kvm -vnc :0 -device edu -monitor stdio

We will get a crash with following asan output:

(qemu) /home/test/qemu5/qemu/hw/intc/ioapic.c:266:27: runtime error: index 35 out of bounds for type 'int [24]'

Comments

Peter Xu June 24, 2019, 9:42 a.m. UTC | #1 
On Fri, Jun 21, 2019 at 05:21:19PM -0700, Li Qiang wrote:
> When emulating irqchip in qemu, such as following command:
> 
> x86_64-softmmu/qemu-system-x86_64 -m 1024 -smp 4 -hda /home/test/test.img
> -machine kernel-irqchip=off --enable-kvm -vnc :0 -device edu -monitor stdio
> 
> We will get a crash with following asan output:
> 
> (qemu) /home/test/qemu5/qemu/hw/intc/ioapic.c:266:27: runtime error: index 35 out of bounds for type 'int [24]'
> =================================================================
> ==113504==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61b000003114 at pc 0x5579e3c7a80f bp 0x7fd004bf8c10 sp 0x7fd004bf8c00
> WRITE of size 4 at 0x61b000003114 thread T4
>     #0 0x5579e3c7a80e in ioapic_eoi_broadcast /home/test/qemu5/qemu/hw/intc/ioapic.c:266
>     #1 0x5579e3c6f480 in apic_eoi /home/test/qemu5/qemu/hw/intc/apic.c:428
>     #2 0x5579e3c720a7 in apic_mem_write /home/test/qemu5/qemu/hw/intc/apic.c:802
>     #3 0x5579e3b1e31a in memory_region_write_accessor /home/test/qemu5/qemu/memory.c:503
>     #4 0x5579e3b1e6a2 in access_with_adjusted_size /home/test/qemu5/qemu/memory.c:569
>     #5 0x5579e3b28d77 in memory_region_dispatch_write /home/test/qemu5/qemu/memory.c:1497
>     #6 0x5579e3a1b36b in flatview_write_continue /home/test/qemu5/qemu/exec.c:3323
>     #7 0x5579e3a1b633 in flatview_write /home/test/qemu5/qemu/exec.c:3362
>     #8 0x5579e3a1bcb1 in address_space_write /home/test/qemu5/qemu/exec.c:3452
>     #9 0x5579e3a1bd03 in address_space_rw /home/test/qemu5/qemu/exec.c:3463
>     #10 0x5579e3b8b979 in kvm_cpu_exec /home/test/qemu5/qemu/accel/kvm/kvm-all.c:2045
>     #11 0x5579e3ae4499 in qemu_kvm_cpu_thread_fn /home/test/qemu5/qemu/cpus.c:1287
>     #12 0x5579e4cbdb9f in qemu_thread_start util/qemu-thread-posix.c:502
>     #13 0x7fd0146376da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
>     #14 0x7fd01436088e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e
> 
> This is because in ioapic_eoi_broadcast function, we uses 'vector' to
> index the 's->irq_eoi'. To fix this, we should uses the irq number.
> 
> Signed-off-by: Li Qiang <liq3ea@163.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

Maybe also add:

Fixes: 958a01dab8 ("ioapic: allow buggy guests mishandling ...")

Should we better clear irq_eoi when the entries are updated in
ioapic_mem_write()?

Regards,
Li Qiang June 24, 2019, 11:08 a.m. UTC | #2 
Peter Xu <peterx@redhat.com> 于2019年6月24日周一 下午5:42写道：

> On Fri, Jun 21, 2019 at 05:21:19PM -0700, Li Qiang wrote:
> > When emulating irqchip in qemu, such as following command:
> >
> > x86_64-softmmu/qemu-system-x86_64 -m 1024 -smp 4 -hda /home/test/test.img
> > -machine kernel-irqchip=off --enable-kvm -vnc :0 -device edu -monitor
> stdio
> >
> > We will get a crash with following asan output:
> >
> > (qemu) /home/test/qemu5/qemu/hw/intc/ioapic.c:266:27: runtime error:
> index 35 out of bounds for type 'int [24]'
> > =================================================================
> > ==113504==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61b000003114 at pc 0x5579e3c7a80f bp 0x7fd004bf8c10 sp 0x7fd004bf8c00
> > WRITE of size 4 at 0x61b000003114 thread T4
> >     #0 0x5579e3c7a80e in ioapic_eoi_broadcast
> /home/test/qemu5/qemu/hw/intc/ioapic.c:266
> >     #1 0x5579e3c6f480 in apic_eoi
> /home/test/qemu5/qemu/hw/intc/apic.c:428
> >     #2 0x5579e3c720a7 in apic_mem_write
> /home/test/qemu5/qemu/hw/intc/apic.c:802
> >     #3 0x5579e3b1e31a in memory_region_write_accessor
> /home/test/qemu5/qemu/memory.c:503
> >     #4 0x5579e3b1e6a2 in access_with_adjusted_size
> /home/test/qemu5/qemu/memory.c:569
> >     #5 0x5579e3b28d77 in memory_region_dispatch_write
> /home/test/qemu5/qemu/memory.c:1497
> >     #6 0x5579e3a1b36b in flatview_write_continue
> /home/test/qemu5/qemu/exec.c:3323
> >     #7 0x5579e3a1b633 in flatview_write /home/test/qemu5/qemu/exec.c:3362
> >     #8 0x5579e3a1bcb1 in address_space_write
> /home/test/qemu5/qemu/exec.c:3452
> >     #9 0x5579e3a1bd03 in address_space_rw
> /home/test/qemu5/qemu/exec.c:3463
> >     #10 0x5579e3b8b979 in kvm_cpu_exec
> /home/test/qemu5/qemu/accel/kvm/kvm-all.c:2045
> >     #11 0x5579e3ae4499 in qemu_kvm_cpu_thread_fn
> /home/test/qemu5/qemu/cpus.c:1287
> >     #12 0x5579e4cbdb9f in qemu_thread_start util/qemu-thread-posix.c:502
> >     #13 0x7fd0146376da in start_thread
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
> >     #14 0x7fd01436088e in __clone
> (/lib/x86_64-linux-gnu/libc.so.6+0x12188e
> >
> > This is because in ioapic_eoi_broadcast function, we uses 'vector' to
> > index the 's->irq_eoi'. To fix this, we should uses the irq number.
> >
> > Signed-off-by: Li Qiang <liq3ea@163.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
>
Thanks Peter.



> Maybe also add:
>
> Fixes: 958a01dab8 ("ioapic: allow buggy guests mishandling ...")
>
> Should we better clear irq_eoi when the entries are updated in
> ioapic_mem_write()?
>
>
Do you mean the redirect table entry of ioapic update?
I think this is reasonable, I will prepare a separate patch with this one
as a patchset later.

Thanks,
Li Qiang



> Regards,
>
> --
> Peter Xu
>
Peter Xu June 24, 2019, 11:20 a.m. UTC | #3 
On Mon, Jun 24, 2019 at 07:08:30PM +0800, Li Qiang wrote:
> Do you mean the redirect table entry of ioapic update?

Yes.

> I think this is reasonable, I will prepare a separate patch with this one
> as a patchset later.

IMHO you can post that as separate patch.  After all these are
different issues and this one looks good already, so no need to
repost.

Regards,
Li Qiang June 24, 2019, 11:24 a.m. UTC | #4 
Peter Xu <peterx@redhat.com> 于2019年6月24日周一 下午7:21写道：

> On Mon, Jun 24, 2019 at 07:08:30PM +0800, Li Qiang wrote:
> > Do you mean the redirect table entry of ioapic update?
>
> Yes.
>
> > I think this is reasonable, I will prepare a separate patch with this one
> > as a patchset later.
>
> IMHO you can post that as separate patch.  After all these are
> different issues and this one looks good already, so no need to
> repost.
>
>

OK, got.

For the maintainer please remember to add the Fixes tag Peter adds in the
review
and also merge the v2 version.


Thanks,
Li Qiang



> Regards,
>
> --
> Peter Xu
>
Li Qiang June 28, 2019, 2:22 a.m. UTC | #5 
Ping, what's the status of this patch.


Li Qiang <liq3ea@163.com> 于2019年6月22日周六 上午8:21写道：

> When emulating irqchip in qemu, such as following command:
>
> x86_64-softmmu/qemu-system-x86_64 -m 1024 -smp 4 -hda /home/test/test.img
> -machine kernel-irqchip=off --enable-kvm -vnc :0 -device edu -monitor stdio
>
> We will get a crash with following asan output:
>
> (qemu) /home/test/qemu5/qemu/hw/intc/ioapic.c:266:27: runtime error: index
> 35 out of bounds for type 'int [24]'
> =================================================================
> ==113504==ERROR: AddressSanitizer: heap-buffer-overflow on address
> 0x61b000003114 at pc 0x5579e3c7a80f bp 0x7fd004bf8c10 sp 0x7fd004bf8c00
> WRITE of size 4 at 0x61b000003114 thread T4
>     #0 0x5579e3c7a80e in ioapic_eoi_broadcast
> /home/test/qemu5/qemu/hw/intc/ioapic.c:266
>     #1 0x5579e3c6f480 in apic_eoi /home/test/qemu5/qemu/hw/intc/apic.c:428
>     #2 0x5579e3c720a7 in apic_mem_write
> /home/test/qemu5/qemu/hw/intc/apic.c:802
>     #3 0x5579e3b1e31a in memory_region_write_accessor
> /home/test/qemu5/qemu/memory.c:503
>     #4 0x5579e3b1e6a2 in access_with_adjusted_size
> /home/test/qemu5/qemu/memory.c:569
>     #5 0x5579e3b28d77 in memory_region_dispatch_write
> /home/test/qemu5/qemu/memory.c:1497
>     #6 0x5579e3a1b36b in flatview_write_continue
> /home/test/qemu5/qemu/exec.c:3323
>     #7 0x5579e3a1b633 in flatview_write /home/test/qemu5/qemu/exec.c:3362
>     #8 0x5579e3a1bcb1 in address_space_write
> /home/test/qemu5/qemu/exec.c:3452
>     #9 0x5579e3a1bd03 in address_space_rw /home/test/qemu5/qemu/exec.c:3463
>     #10 0x5579e3b8b979 in kvm_cpu_exec
> /home/test/qemu5/qemu/accel/kvm/kvm-all.c:2045
>     #11 0x5579e3ae4499 in qemu_kvm_cpu_thread_fn
> /home/test/qemu5/qemu/cpus.c:1287
>     #12 0x5579e4cbdb9f in qemu_thread_start util/qemu-thread-posix.c:502
>     #13 0x7fd0146376da in start_thread
> (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
>     #14 0x7fd01436088e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e
>
> This is because in ioapic_eoi_broadcast function, we uses 'vector' to
> index the 's->irq_eoi'. To fix this, we should uses the irq number.
>
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
> Change since v1:
> remove auto-generated unnecessary message
>
>  hw/intc/ioapic.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
> index 7074489fdf..711775cc6f 100644
> --- a/hw/intc/ioapic.c
> +++ b/hw/intc/ioapic.c
> @@ -245,8 +245,8 @@ void ioapic_eoi_broadcast(int vector)
>              s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
>
>              if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
> -                ++s->irq_eoi[vector];
> -                if (s->irq_eoi[vector] >= SUCCESSIVE_IRQ_MAX_COUNT) {
> +                ++s->irq_eoi[n];
> +                if (s->irq_eoi[n] >= SUCCESSIVE_IRQ_MAX_COUNT) {
>                      /*
>                       * Real hardware does not deliver the interrupt
> immediately
>                       * during eoi broadcast, and this lets a buggy guest
> make
> @@ -254,16 +254,16 @@ void ioapic_eoi_broadcast(int vector)
>                       * level-triggered interrupt. Emulate this behavior
> if we
>                       * detect an interrupt storm.
>                       */
> -                    s->irq_eoi[vector] = 0;
> +                    s->irq_eoi[n] = 0;
>                      timer_mod_anticipate(s->delayed_ioapic_service_timer,
>
> qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
>                                           NANOSECONDS_PER_SECOND / 100);
> -                    trace_ioapic_eoi_delayed_reassert(vector);
> +                    trace_ioapic_eoi_delayed_reassert(n);
>                  } else {
>                      ioapic_service(s);
>                  }
>              } else {
> -                s->irq_eoi[vector] = 0;
> +                s->irq_eoi[n] = 0;
>              }
>          }
>      }
> --
> 2.17.1
>
>
>
diff mbox series

Patch

=================================================================
==113504==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61b000003114 at pc 0x5579e3c7a80f bp 0x7fd004bf8c10 sp 0x7fd004bf8c00
WRITE of size 4 at 0x61b000003114 thread T4
    #0 0x5579e3c7a80e in ioapic_eoi_broadcast /home/test/qemu5/qemu/hw/intc/ioapic.c:266
    #1 0x5579e3c6f480 in apic_eoi /home/test/qemu5/qemu/hw/intc/apic.c:428
    #2 0x5579e3c720a7 in apic_mem_write /home/test/qemu5/qemu/hw/intc/apic.c:802
    #3 0x5579e3b1e31a in memory_region_write_accessor /home/test/qemu5/qemu/memory.c:503
    #4 0x5579e3b1e6a2 in access_with_adjusted_size /home/test/qemu5/qemu/memory.c:569
    #5 0x5579e3b28d77 in memory_region_dispatch_write /home/test/qemu5/qemu/memory.c:1497
    #6 0x5579e3a1b36b in flatview_write_continue /home/test/qemu5/qemu/exec.c:3323
    #7 0x5579e3a1b633 in flatview_write /home/test/qemu5/qemu/exec.c:3362
    #8 0x5579e3a1bcb1 in address_space_write /home/test/qemu5/qemu/exec.c:3452
    #9 0x5579e3a1bd03 in address_space_rw /home/test/qemu5/qemu/exec.c:3463
    #10 0x5579e3b8b979 in kvm_cpu_exec /home/test/qemu5/qemu/accel/kvm/kvm-all.c:2045
    #11 0x5579e3ae4499 in qemu_kvm_cpu_thread_fn /home/test/qemu5/qemu/cpus.c:1287
    #12 0x5579e4cbdb9f in qemu_thread_start util/qemu-thread-posix.c:502
    #13 0x7fd0146376da in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x76da)
    #14 0x7fd01436088e in __clone (/lib/x86_64-linux-gnu/libc.so.6+0x12188e

This is because in ioapic_eoi_broadcast function, we uses 'vector' to
index the 's->irq_eoi'. To fix this, we should uses the irq number.

Signed-off-by: Li Qiang <liq3ea@163.com>
---
Change since v1:
remove auto-generated unnecessary message

 hw/intc/ioapic.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
index 7074489fdf..711775cc6f 100644
--- a/hw/intc/ioapic.c
+++ b/hw/intc/ioapic.c
@@ -245,8 +245,8 @@  void ioapic_eoi_broadcast(int vector)
             s->ioredtbl[n] = entry & ~IOAPIC_LVT_REMOTE_IRR;
 
             if (!(entry & IOAPIC_LVT_MASKED) && (s->irr & (1 << n))) {
-                ++s->irq_eoi[vector];
-                if (s->irq_eoi[vector] >= SUCCESSIVE_IRQ_MAX_COUNT) {
+                ++s->irq_eoi[n];
+                if (s->irq_eoi[n] >= SUCCESSIVE_IRQ_MAX_COUNT) {
                     /*
                      * Real hardware does not deliver the interrupt immediately
                      * during eoi broadcast, and this lets a buggy guest make
@@ -254,16 +254,16 @@  void ioapic_eoi_broadcast(int vector)
                      * level-triggered interrupt. Emulate this behavior if we
                      * detect an interrupt storm.
                      */
-                    s->irq_eoi[vector] = 0;
+                    s->irq_eoi[n] = 0;
                     timer_mod_anticipate(s->delayed_ioapic_service_timer,
                                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
                                          NANOSECONDS_PER_SECOND / 100);
-                    trace_ioapic_eoi_delayed_reassert(vector);
+                    trace_ioapic_eoi_delayed_reassert(n);
                 } else {
                     ioapic_service(s);
                 }
             } else {
-                s->irq_eoi[vector] = 0;
+                s->irq_eoi[n] = 0;
             }
         }
     }