| Message ID | 20201011082022.3016-1-mark.cave-ayland@ilande.co.uk |
|---|---|
| State | New |
| Headers | show |
| Series | pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level() | expand |
On 11/10/2020 09:20, Mark Cave-Ayland wrote: > These assertions similar to those in the adjacent pci_bus_get_irq_level() function > ensure that irqnum lies within the valid PCI bus IRQ range. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > > This would have immediately picked up on the sabre PCI bus IRQ overflow fixed by > the patch I just posted. > > --- > hw/pci/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 3c8f10b461..b1484b3747 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -258,6 +258,8 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > 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, irq_num, bus->irq_count[irq_num] != 0); > } Actually something else is odd here: I've just done a quick check on the callers to pci_change_irq_level() and it appears that both pci_update_irq_disabled() and pci_irq_handler() assume that irqnum is a PCI device IRQ i.e between 0 and 3, whereas pci_change_irq_level() assumes it is working with a PCI bus IRQ between 0 and bus->nirqs. It feels like pci_change_irq_level() should be renamed to pci_bus_change_irq_level() similar to pci_bus_get_irq_level() but in that case are pci_update_irq_disabled() and pci_irq_handler() both incorrect? ATB, Mark.
On 11/10/2020 09:20, Mark Cave-Ayland wrote: > These assertions similar to those in the adjacent pci_bus_get_irq_level() function > ensure that irqnum lies within the valid PCI bus IRQ range. > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> > --- > > This would have immediately picked up on the sabre PCI bus IRQ overflow fixed by > the patch I just posted. > > --- > hw/pci/pci.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c > index 3c8f10b461..b1484b3747 100644 > --- a/hw/pci/pci.c > +++ b/hw/pci/pci.c > @@ -258,6 +258,8 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) > 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, irq_num, bus->irq_count[irq_num] != 0); > } Ping? ATB, Mark.
+Igor/Julia On 10/11/20 10:27 AM, Mark Cave-Ayland wrote: > On 11/10/2020 09:20, Mark Cave-Ayland wrote: > >> These assertions similar to those in the adjacent pci_bus_get_irq_level() function >> ensure that irqnum lies within the valid PCI bus IRQ range. >> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> >> --- >> >> This would have immediately picked up on the sabre PCI bus IRQ overflow fixed by >> the patch I just posted. >> >> --- >> hw/pci/pci.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c >> index 3c8f10b461..b1484b3747 100644 >> --- a/hw/pci/pci.c >> +++ b/hw/pci/pci.c >> @@ -258,6 +258,8 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) >> 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, irq_num, bus->irq_count[irq_num] != 0); >> } > > Actually something else is odd here: I've just done a quick check on the callers to > pci_change_irq_level() and it appears that both pci_update_irq_disabled() and > pci_irq_handler() assume that irqnum is a PCI device IRQ i.e between 0 and 3, whereas > pci_change_irq_level() assumes it is working with a PCI bus IRQ between 0 and bus->nirqs. IIUC pci_map_irq_fn() returns [0..3] (PCI_NUM_PINS). > > It feels like pci_change_irq_level() should be renamed to pci_bus_change_irq_level() I don't think so, maybe extracted? -- >8 -- diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 100c9381c2f..79fb94394cc 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -248,6 +248,12 @@ static inline void pci_set_irq_state(PCIDevice *d, int irq_num, int level) d->irq_state |= level << irq_num; } +static void pci_bus_change_irq_level(PCIBus *bus, int irq_num, int change) +{ + assert(irq_num >= 0); + assert(irq_num < bus->nirq); + bus->irq_count[irq_num] += change; + bus->set_irq(bus->irq_opaque, irq_num, bus->irq_count[irq_num] != 0); +} + static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) { PCIBus *bus; @@ -258,8 +264,7 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) 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); + pci_bus_change_irq_level(bus, irq_num, change); } --- > similar to pci_bus_get_irq_level() but in that case are pci_update_irq_disabled() and > pci_irq_handler() both incorrect? > > > ATB, > > Mark. >
diff --git a/hw/pci/pci.c b/hw/pci/pci.c index 3c8f10b461..b1484b3747 100644 --- a/hw/pci/pci.c +++ b/hw/pci/pci.c @@ -258,6 +258,8 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change) 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, irq_num, bus->irq_count[irq_num] != 0); }
These assertions similar to those in the adjacent pci_bus_get_irq_level() function ensure that irqnum lies within the valid PCI bus IRQ range. Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> --- This would have immediately picked up on the sabre PCI bus IRQ overflow fixed by the patch I just posted. --- hw/pci/pci.c | 2 ++ 1 file changed, 2 insertions(+)