[Qemu-devel] pci_change_irq_level is broken...

[Alan Amaral]

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].  The problem is that in the loop above where

bus->irq_count[irq_num] is set, the value if irq_num is changed:

 

        irq_num = bus->map_irq(pci_dev, irq_num);

and I've seen the value of irq_num go as high as 24.  Since irq_count has only 4 elements,

the code is running off the end of the array.   Luckily, so far, change has always been 0,

so doing the += change has had no effect, but it is clearly wrong.  Here's the original code:

static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
{
    PCIBus *bus;
    for (;;) {
        bus = pci_dev->bus;
        irq_num = bus->map_irq(pci_dev, irq_num);
        if (bus->set_irq)
            break;
        pci_dev = bus->parent_dev;
    }
    bus->irq_count[irq_num] += change;
    bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0);
}

 

I believe that the code should look like this instead:

 

static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
{
    PCIBus *bus;

    int bus_irq_num;
    for (;;) {
        bus = pci_dev->bus;
        bus_irq_num = bus->map_irq(pci_dev, irq_num);
        if (bus->set_irq)
            break;
        pci_dev = bus->parent_dev;
    }
    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);
}

 

Although this version may actually be correct if the irq needs to be remapped for each

cascaded bus:

 

static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
{
    PCIBus *bus;

    int bus_irq_num = irq_num;

    for (;;) {
        bus = pci_dev->bus;
        bus_irq_num = bus->map_irq(pci_dev, bus_irq_num);
        if (bus->set_irq)
            break;
        pci_dev = bus->parent_dev;
    }

    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);
}

 

Thanks,

 

Alan Amaral

Virtual Computer

address@hidden