[PATCH] pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level()

[PATCH] pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level()

[Mark Cave-Ayland]

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

Re: [PATCH] pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level()

[Mark Cave-Ayland]

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.

Re: [PATCH] pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level()

[Mark Cave-Ayland]

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.

Re: [PATCH] pci: assert that irqnum is between 0 and bus->nirqs in pci_change_irq_level()

[Philippe Mathieu-Daudé]

+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.
>