[Qemu-devel] pci_change_irq_level is broken...

[Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

[-- Attachment #1: Type: text/html, Size: 6362 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Richard Henderson]

On 09/20/2011 10:24 AM, Alan Amaral wrote:
> I'm not on this mailing list, so please CC me on any replies.  Thanks.
>  
> I ran qemu with valgrind last night and found an error in the pci emulation code, which may,
> or may not, be biting us.  So far the effects seem benign, although there exists the possibility
> of trashing random memory.
>  
> In the function pci_change_irq_level() the argument irq_num is passed in as 0-3, and used
> as an index to change bus->irq_count[4].

I don't know what version of qemu you're using, but this is

      int *irq_count;

in current sources.  There's certainly no hard-coded "4".

>     assert(irq_num >= 0);
>     assert(irq_num < bus->nirq);
>     bus->irq_count[irq_num] += change;
>     bus->set_irq(bus->irq_opaque, bus_irq_num, bus->irq_count[irq_num] != 0);

This version with the asserts, though, could be done.  The site
that created the bus ought to match up nirq with the map function.


r~

Re: [Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard

You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
there's a comment just above pci_set_irq which says:

/* 0 <= irq_num <= 3. level must be 0 or 1 */
static void pci_set_irq(void *opaque, int irq_num, int level)

so, that implies to me that it's probably always 4...  Sorry for the confusion.



From: Richard Henderson
Sent: Tue 9/20/2011 2:57 PM
To: Alan Amaral
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] pci_change_irq_level is broken...


On 09/20/2011 10:24 AM, Alan Amaral wrote:
> I'm not on this mailing list, so please CC me on any replies.  Thanks.
>  
> I ran qemu with valgrind last night and found an error in the pci emulation code, which may,
> or may not, be biting us.  So far the effects seem benign, although there exists the possibility
> of trashing random memory.
>  
> In the function pci_change_irq_level() the argument irq_num is passed in as 0-3, and used
> as an index to change bus->irq_count[4].

I don't know what version of qemu you're using, but this is

      int *irq_count;

in current sources.  There's certainly no hard-coded "4".

>     assert(irq_num >= 0);
>     assert(irq_num < bus->nirq);
>     bus->irq_count[irq_num] += change;
>     bus->set_irq(bus->irq_opaque, bus_irq_num, bus->irq_count[irq_num] != 0);

This version with the asserts, though, could be done.  The site
that created the bus ought to match up nirq with the map function.


r~

[-- Attachment #2: Type: text/html, Size: 2292 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 840 bytes --]

On 2011-09-20 21:19, Alan Amaral wrote:
> QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard

(That's an ambitious development version.)

> 
> You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
> there's a comment just above pci_set_irq which says:
> 
> /* 0 <= irq_num <= 3. level must be 0 or 1 */
> static void pci_set_irq(void *opaque, int irq_num, int level)
> 
> so, that implies to me that it's probably always 4...  Sorry for the confusion.

Assuming you look at PIIX3: Yes, it allocates 4 IRQs - but only returns
0..3 via pci_slot_get_pirq. Xen uses some more, but also looks safe.

Can you provide a backtrace where irq_num gets larger than 3 and writes
beyond the end of irq_count? Do you have private patches in your tree?

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Richard Henderson]

On 09/20/2011 12:19 PM, Alan Amaral wrote:
> QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard
> You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
> there's a comment just above pci_set_irq which says:
>  
> /* 0 <= irq_num <= 3. level must be 0 or 1 */
> static void pci_set_irq(void *opaque, int irq_num, int level)
> so, that implies to me that it's probably always 4...

The first use I examined was apb_pci.c:

    d->bus = pci_register_bus(&d->busdev.qdev, "pci",
                              pci_apb_set_irq, pci_pbm_map_irq, d,
                              &d->pci_mmio,
                              get_system_io(),
                              0, 32);

where the last argument indicates that we allocate 32 irqs, 
and the pci_pbm_map_irq function returns a value in the set
{ 0, 1, 2, 3, 16, 17, 18, 19 }.


r~

Re: [Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

[-- Attachment #1: Type: text/plain, Size: 1752 bytes --]

Ok.  Cases to the contrary notwithstanding, what I'm seeing is the size of the irq_count array
is 4 and the code is clearly accessing entry 4 and beyond.  We are using Xen (sorry if I didn't
mention that earlier).

#0  pci_change_irq_level (pci_dev=0x1c3a730, irq_num=4, change=0)
    at /home/aamaral/orc-next/orc-tree/orc-xen/xen-4.0/tools/ioemu-dir/hw/pci.c:125
125     bus->irq_count[irq_num] += change;
(gdb) p irq_num
$23 = 4                <- It was zero on entry to this function but was changed by xen_pci_slot_get_pirq
(gdb) p bus->nirq 
$24 = 4
(gdb)

Please see my earlier reply to Jan Kiszka for full details.

Thanks,

Alan



From: Richard Henderson
Sent: Tue 9/20/2011 3:56 PM
To: Alan Amaral
Cc: qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] pci_change_irq_level is broken...


On 09/20/2011 12:19 PM, Alan Amaral wrote:
> QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard
> You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
> there's a comment just above pci_set_irq which says:
>  
> /* 0 <= irq_num <= 3. level must be 0 or 1 */
> static void pci_set_irq(void *opaque, int irq_num, int level)
> so, that implies to me that it's probably always 4...

The first use I examined was apb_pci.c:

    d->bus = pci_register_bus(&d->busdev.qdev, "pci",
                              pci_apb_set_irq, pci_pbm_map_irq, d,
                              &d->pci_mmio,
                              get_system_io(),
                              0, 32);

where the last argument indicates that we allocate 32 irqs, 
and the pci_pbm_map_irq function returns a value in the set
{ 0, 1, 2, 3, 16, 17, 18, 19 }.


r~

[-- Attachment #2: Type: text/html, Size: 2813 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

[-- Attachment #1: Type: text/plain, Size: 8966 bytes --]

  



From: Jan Kiszka
Sent: Tue 9/20/2011 3:41 PM
To: Alan Amaral
Cc: Richard Henderson; qemu-devel@nongnu.org
Subject: Re: pci_change_irq_level is broken...

> On 2011-09-20 21:19, Alan Amaral wrote:
> > QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard
> 
> (That's an ambitious development version.)

It's what we're using.

> > 
> > You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
> > there's a comment just above pci_set_irq which says:
> > 
> > /* 0 <= irq_num <= 3. level must be 0 or 1 */
> > static void pci_set_irq(void *opaque, int irq_num, int level)
> > 
> > so, that implies to me that it's probably always 4...  Sorry for the confusion.
> 
> Assuming you look at PIIX3: Yes, it allocates 4 IRQs - but only returns
> 0..3 via pci_slot_get_pirq. Xen uses some more, but also looks safe.

We are running under Xen and in this case it is using PIIX_NUM_PIRQS,
which is 4, as the last arg to pci_bus_irqs().  

PCIBus *i440fx_xen_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
                        qemu_irq *pic, ram_addr_t ram_size)
{
    PCIBus *b;
    b = i440fx_common_init("i440FX-xen", pi440fx_state, piix3_devfn, pic, ram_size);
    pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
                 (*pi440fx_state)->piix3, PIIX_NUM_PIRQS);
    return b;
}

Also, since we are using xen, xen_pci_slot_get_pirq is being used, which always returns
something >= 4 (at least when pci_dev->devfn > 0).  Here's the code:

int xen_pci_slot_get_pirq(PCIDevice *pci_dev, int irq_num)
{
    return irq_num + ((pci_dev->devfn >> 3) << 2);
}

In the case I'm seeing devfn == 9.  I'm in gdb now, and at a breakpoint at the error condition.
The call is:

Breakpoint 1, pci_change_irq_level (pci_dev=0x1c3a730, irq_num=0, change=0)

(gdb) p *pci_dev
$1 = {qdev = {id = 0x0, state = DEV_STATE_INITIALIZED, opts = 0x0, 
    hotplugged = 0, info = 0xa08c00, parent_bus = 0x1af5700, num_gpio_out = 0, 
    gpio_out = 0x0, num_gpio_in = 0, gpio_in = 0x0, child_bus = {
      lh_first = 0x1c3b0c8}, num_child_bus = 2, sibling = {
      le_next = 0x1c37440, le_prev = 0x1c9dcb0}, instance_id_alias = -1, 
    alias_required_for_version = 0}, config = 0x1c3b8e0 "\206\200\020p", 
  cmask = 0x1c3b9f0 "\377\377\377\377", wmask = 0x1c3bb00 "", 
  w1cmask = 0x1c3bc10 "", used = 0x1c3bd20 "", bus = 0x1af5700, devfn = 9, 
  name = "piix3-ide", '\000' <repeats 54 times>, io_regions = {{addr = 0, 
      size = 0, filtered_size = 0, type = 0 '\000', map_func = 0, 
      ram_addr = 0}, {addr = 0, size = 0, filtered_size = 0, type = 0 '\000', 
      map_func = 0, ram_addr = 0}, {addr = 0, size = 0, filtered_size = 0, 
      type = 0 '\000', map_func = 0, ram_addr = 0}, {addr = 0, size = 0, 
      filtered_size = 0, type = 0 '\000', map_func = 0, ram_addr = 0}, {
      addr = 18446744073709551615, size = 16, filtered_size = 16, 
      type = 1 '\001', map_func = 0x60095c <bmdma_map>, ram_addr = 16}, {
      addr = 0, size = 0, filtered_size = 0, type = 0 '\000', map_func = 0, 
      ram_addr = 0}, {addr = 0, size = 0, filtered_size = 0, type = 0 '\000', 
      map_func = 0, ram_addr = 0}}, 
  config_read = 0x5be0c9 <pci_default_read_config>, 
  config_write = 0x5be192 <pci_default_write_config>, irq = 0x1c3be30, 
  irq_state = 0 '\000', cap_present = 16, msix_cap = 0 '\000', 
  msix_entries_nr = 0, msix_table_page = 0x0, msix_mmio_index = 0, 
  msix_entry_used = 0x0, msix_bar_size = 0, version_id = 2, 
  msi_cap = 0 '\000', exp = {exp_cap = 0 '\000', hpev_intx = 0, 
    hpev_notified = false, aer_cap = 0, aer_log = {log_num = 0, log_max = 0, 
      log = 0x0}, aer_intx = 0}, romfile = 0x0, rom_offset = 0, rom_bar = 1}

(gdb) p *bus
$2 = {qbus = {parent = 0x1af41b0, info = 0x9e4e60, name = 0x1736910 "pci.0", 
    allow_hotplug = 1, qdev_allocated = 1, children = {lh_first = 0x1cd3520}, 
    sibling = {le_next = 0x0, le_prev = 0x1af4200}}, devfn_min = 0 '\000', 
  set_irq = 0x62e600 <xen_piix3_set_irq>, 
  map_irq = 0x62e5b2 <xen_pci_slot_get_pirq>, 
  hotplug = 0x5d71b8 <piix4_device_hotplug>, hotplug_qdev = 0x1cd1600, 
  irq_opaque = 0x1af6fd0, devices = {0x1af6150, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
    0x0, 0x1af6fd0, 0x1c3a730, 0x1cd09c0, 0x1cd1600, 0x0, 0x0, 0x0, 0x0, 
    0x1af7be0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1c37440, 0x0, 0x0, 0x0, 
    0x0, 0x0, 0x0, 0x0, 0x1cbe010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
    0x1c9dc50, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1cd3520, 
    0x0 <repeats 207 times>}, parent_dev = 0x0, mem_base = 0, child = {
    lh_first = 0x0}, sibling = {le_next = 0x0, le_prev = 0x0}, nirq = 4, 
  irq_count = 0x1af7ab0}

(gdb) p bus->nirq     
$3 = 4
(gdb) p pci_dev->devfn
$4 = 9
(gdb) p bus->map_irq
$5 = (pci_map_irq_fn) 0x62e5b2 <xen_pci_slot_get_pirq>

After the first call to map_irq irq_num is changed to 4:

(gdb) n
119         bus = pci_dev->bus;
(gdb) 
120         irq_num = bus->map_irq(pci_dev, irq_num);
(gdb) 
121         if (bus->set_irq)
(gdb) p irq_num
$6 = 4
(gdb) p bus->set_irq 
$7 = (pci_set_irq_fn) 0x62e600 <xen_piix3_set_irq>
(gdb) n
125     bus->irq_count[irq_num] += change;
(gdb) p irq_num
$8 = 4
(gdb) p bus->nirq 
$9 = 4
(gdb) whatis bus->irq_count[0] 
type = int
(gdb) p sizeof(bus->irq_count[0])
$10 = 4
(gdb) 

This all clearly shows that the irq_count array has 4 elements and the code
is trying to read and write irq_count[4], which is outside of the malloc'd block.

> Can you provide a backtrace where irq_num gets larger than 3 and writes
> beyond the end of irq_count? Do you have private patches in your tree?

Backtrace is below.  Oh, yes, we're using private patches, but I don't believe
that this code has been patched at all.

> Jan
> 

Here's a stack trace from valgrind.  You'll note that the allocated block size is 16,
which is 4 ints, and I verified above in gdb that bus->irq_nirq was 4.
 ==1901====1901== 52 errors in context 17 of 125:==1901== Thread 1:==1901== Invalid read of size 4==1901==    at 0x5BB7FB: pci_change_irq_level (pci.c:126)==1901==    by 0x5BE021: pci_update_irq_disabled (pci.c:1083)==1901==    by 0x5BE303: pci_default_write_config (pci.c:1116)==1901==    by 0x5C4724: pci_data_write (pci_host.c:60)==1901==    by 0x5C4923: pci_host_data_write (pci_host.c:109)==1901==    by 0x428B3B: ioport_simple_writew (rwhandler.c:50)==1901==    by 0x481EF0: ioport_write (ioport.c:81)==1901==    by 0x48285B: cpu_outw (ioport.c:273)==1901==    by 0x62ED59: do_outp (xen-all.c:307)==1901==    by 0x62EEDA: cpu_ioreq_pio (xen-all.c:335)==1901==    by 0x62F28E: handle_ioreq (xen-all.c:396)==1901==    by 0x62F5A6: cpu_handle_ioreq (xen-all.c:464)==1901==    by 0x4BD078: qemu_iohandler_poll (iohandler.c:120)==1901==    by 0x5AE4F2: main_loop_wait (vl.c:1359)==1901==    by 0x5AE5F7: main_loop (vl.c:1404)==1901==    by 0x5B2FBC: main (vl.c:3436)==1901==  Address 0x2b7f9d30 is 0 bytes after a block of size 16 alloc'd==1901==    at 0x4C279FC: calloc (vg_replace_malloc.c:467)==1901==    by 0x42A6B5: qemu_mallocz (qemu-malloc.c:71)==1901==    by 0x5BC01E: pci_bus_irqs (pci.c:296)==1901==    by 0x66D6B7: i440fx_xen_init (piix_pci.c:304)==1901==    by 0x670D88: pc_init1 (pc_piix.c:135)==1901==    by 0x671279: pc_init_pci_no_kvmclock (pc_piix.c:236)==1901==    by 0x671385: pc_xen_hvm_init (pc_piix.c:266)==1901==    by 0x5B2C01: main (vl.c:3281)==1901====1901== 52 errors in context 18 of 125:==1901== Invalid write of size 4==1901==    at 0x5BB7D9: pci_change_irq_level (pci.c:125)==1901==    by 0x5BE021: pci_update_irq_disabled (pci.c:1083)==1901==    by 0x5BE303: pci_default_write_config (pci.c:1116)==1901==    by 0x5C4724: pci_data_write (pci_host.c:60)==1901==    by 0x5C4923: pci_host_data_write (pci_host.c:109)==1901==    by 0x428B3B: ioport_simple_writew (rwhandler.c:50)==1901==    by 0x481EF0: ioport_write (ioport.c:81)==1901==    by 0x48285B: cpu_outw (ioport.c:273)==1901==    by 0x62ED59: do_outp (xen-all.c:307)==1901==    by 0x62EEDA: cpu_ioreq_pio (xen-all.c:335)==1901==    by 0x62F28E: handle_ioreq (xen-all.c:396)==1901==    by 0x62F5A6: cpu_handle_ioreq (xen-all.c:464)==1901==    by 0x4BD078: qemu_iohandler_poll (iohandler.c:120)==1901==    by 0x5AE4F2: main_loop_wait (vl.c:1359)==1901==    by 0x5AE5F7: main_loop (vl.c:1404)==1901==    by 0x5B2FBC: main (vl.c:3436)==1901==  Address 0x2b7f9d30 is 0 bytes after a block of size 16 alloc'd==1901==    at 0x4C279FC: calloc (vg_replace_malloc.c:467)==1901==    by 0x42A6B5: qemu_mallocz (qemu-malloc.c:71)==1901==    by 0x5BC01E: pci_bus_irqs (pci.c:296)==1901==    by 0x66D6B7: i440fx_xen_init (piix_pci.c:304)==1901==    by 0x670D88: pc_init1 (pc_piix.c:135)==1901==    by 0x671279: pc_init_pci_no_kvmclock (pc_piix.c:236)==1901==    by 0x671385: pc_xen_hvm_init (pc_piix.c:266)==1901==    by 0x5B2C01: main (vl.c:3281)==1901==

[-- Attachment #2: Type: text/html, Size: 12695 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1569 bytes --]

On 2011-09-21 18:26, Alan Amaral wrote:
>   
> 
> 
> 
> From: Jan Kiszka
> Sent: Tue 9/20/2011 3:41 PM
> To: Alan Amaral
> Cc: Richard Henderson; qemu-devel@nongnu.org
> Subject: Re: pci_change_irq_level is broken...
> 
>> On 2011-09-20 21:19, Alan Amaral wrote:
>>> QEMU emulator version 0.14.50, Copyright (c) 2003-2008 Fabrice Bellard
>>
>> (That's an ambitious development version.)
> 
> It's what we're using.
> 
>>>
>>> You are correct, it's not hardcoded to 4.  However, when it's allocated the number of elements IS 4.  Also,
>>> there's a comment just above pci_set_irq which says:
>>>
>>> /* 0 <= irq_num <= 3. level must be 0 or 1 */
>>> static void pci_set_irq(void *opaque, int irq_num, int level)
>>>
>>> so, that implies to me that it's probably always 4...  Sorry for the confusion.
>>
>> Assuming you look at PIIX3: Yes, it allocates 4 IRQs - but only returns
>> 0..3 via pci_slot_get_pirq. Xen uses some more, but also looks safe.
> 
> We are running under Xen and in this case it is using PIIX_NUM_PIRQS,
> which is 4, as the last arg to pci_bus_irqs().  
> 
> PCIBus *i440fx_xen_init(PCII440FXState **pi440fx_state, int *piix3_devfn,
>                         qemu_irq *pic, ram_addr_t ram_size)
> {
>     PCIBus *b;
>     b = i440fx_common_init("i440FX-xen", pi440fx_state, piix3_devfn, pic, ram_size);
>     pci_bus_irqs(b, xen_piix3_set_irq, xen_pci_slot_get_pirq,
>                  (*pi440fx_state)->piix3, PIIX_NUM_PIRQS);
>     return b;
> }

bf09551a6b, aka "xen: fix interrupt routing".

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

Re: [Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

[-- Attachment #1: Type: text/html, Size: 2397 bytes --]