[PATCH] pci: check bus pointer before dereference

[PATCH] pci: check bus pointer before dereference

[P J P]

From: Prasad J Pandit <pjp@fedoraproject.org>

While mapping IRQ level in pci_change_irq_level() routine,
it does not check if pci_get_bus() returned a valid pointer.
It may lead to a NULL pointer dereference issue. Add check to
avoid it.

 -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
    ==1183858==Hint: address points to the zero page.
    #0 pci_change_irq_level hw/pci/pci.c:259
    #1 pci_irq_handler hw/pci/pci.c:1445
    #2 pci_set_irq hw/pci/pci.c:1463
    #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
    #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
    #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
    #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
    #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
    #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
    ...

Reported-by: Ruhr-University <bugs-syssec@rub.de>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
 hw/pci/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index de0fae10ab..df5a2c3294 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -253,6 +253,9 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
     PCIBus *bus;
     for (;;) {
         bus = pci_get_bus(pci_dev);
+        if (!bus) {
+            return;
+        }
         irq_num = bus->map_irq(pci_dev, irq_num);
         if (bus->set_irq)
             break;
-- 
2.26.2

Re: [PATCH] pci: check bus pointer before dereference

[P J P]

+-- On Thu, 27 Aug 2020, P J P wrote --+
| While mapping IRQ level in pci_change_irq_level() routine,
| it does not check if pci_get_bus() returned a valid pointer.
| It may lead to a NULL pointer dereference issue. Add check to
| avoid it.
| 
|  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
|     ==1183858==Hint: address points to the zero page.
|     #0 pci_change_irq_level hw/pci/pci.c:259
|     #1 pci_irq_handler hw/pci/pci.c:1445
|     #2 pci_set_irq hw/pci/pci.c:1463
|     #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
|     #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
|     #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
|     #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
|     #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
|     #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
|     ...
| 
| Reported-by: Ruhr-University <bugs-syssec@rub.de>
| Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
| ---
|  hw/pci/pci.c | 3 +++
|  1 file changed, 3 insertions(+)
| 
| diff --git a/hw/pci/pci.c b/hw/pci/pci.c
| index de0fae10ab..df5a2c3294 100644
| --- a/hw/pci/pci.c
| +++ b/hw/pci/pci.c
| @@ -253,6 +253,9 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
|      PCIBus *bus;
|      for (;;) {
|          bus = pci_get_bus(pci_dev);
| +        if (!bus) {
| +            return;
| +        }
|          irq_num = bus->map_irq(pci_dev, irq_num);
|          if (bus->set_irq)
|              break;
| 


Ping...!
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[Li Qiang]

P J P <ppandit@redhat.com> 于2020年8月27日周四 下午7:52写道：
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While mapping IRQ level in pci_change_irq_level() routine,
> it does not check if pci_get_bus() returned a valid pointer.
> It may lead to a NULL pointer dereference issue. Add check to
> avoid it.
>
>  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
>     ==1183858==Hint: address points to the zero page.
>     #0 pci_change_irq_level hw/pci/pci.c:259
>     #1 pci_irq_handler hw/pci/pci.c:1445
>     #2 pci_set_irq hw/pci/pci.c:1463
>     #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
>     #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
>     #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
>     #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
>     #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
>     #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
>     ...
>
> Reported-by: Ruhr-University <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index de0fae10ab..df5a2c3294 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -253,6 +253,9 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
>      PCIBus *bus;
>      for (;;) {
>          bus = pci_get_bus(pci_dev);
> +        if (!bus) {

Hi Prasad,

I think in normal this 'bus' will be not NULL.
I have look at the link in the commit msg.
I find it is another DMA to MMIO issue which we have discussed a lot
but didn't come up with an
satisfying solution.

Maybe we can try to the DMA to MMIO issue direction.
CC: Peter, Jason and Alex

Thanks,
Li Qiang


> +            return;
> +        }
>          irq_num = bus->map_irq(pci_dev, irq_num);
>          if (bus->set_irq)
>              break;
> --
> 2.26.2
>
>

Re: [PATCH] pci: check bus pointer before dereference

[Philippe Mathieu-Daudé]

+Igor

On 9/15/20 3:51 PM, Li Qiang wrote:
> P J P <ppandit@redhat.com> 于2020年8月27日周四 下午7:52写道：
>>
>> From: Prasad J Pandit <pjp@fedoraproject.org>
>>
>> While mapping IRQ level in pci_change_irq_level() routine,
>> it does not check if pci_get_bus() returned a valid pointer.
>> It may lead to a NULL pointer dereference issue. Add check to
>> avoid it.
>>
>>  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
>>     ==1183858==Hint: address points to the zero page.
>>     #0 pci_change_irq_level hw/pci/pci.c:259
>>     #1 pci_irq_handler hw/pci/pci.c:1445
>>     #2 pci_set_irq hw/pci/pci.c:1463
>>     #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
>>     #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
>>     #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
>>     #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
>>     #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
>>     #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
>>     ...
>>
>> Reported-by: Ruhr-University <bugs-syssec@rub.de>
>> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
>> ---
>>  hw/pci/pci.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
>> index de0fae10ab..df5a2c3294 100644
>> --- a/hw/pci/pci.c
>> +++ b/hw/pci/pci.c
>> @@ -253,6 +253,9 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int irq_num, int change)
>>      PCIBus *bus;
>>      for (;;) {
>>          bus = pci_get_bus(pci_dev);
>> +        if (!bus) {
> 
> Hi Prasad,
> 
> I think in normal this 'bus' will be not NULL.
> I have look at the link in the commit msg.
> I find it is another DMA to MMIO issue which we have discussed a lot
> but didn't come up with an
> satisfying solution.
> 
> Maybe we can try to the DMA to MMIO issue direction.
> CC: Peter, Jason and Alex
> 
> Thanks,
> Li Qiang
> 
> 
>> +            return;

Nack, this should be an abort().

As usual, question is how we got here.

As Li said, it is another DMA to MMIO bug class.

lsi_execute_script
 -> address_space_write
    -> acpi_pcihp_eject_slot
       -> bus_remove_child

So at this point the PCI device is still MMIO-mapped
but eject from the bus... ???
Then IRQ is triggered, which the device wants to
propagate via its PCI bus but it doesn't have any more
and b00m.

If a device is hotpluggable, who is responsible to
unmap its regions?

>> +        }
>>          irq_num = bus->map_irq(pci_dev, irq_num);
>>          if (bus->set_irq)
>>              break;
>> --
>> 2.26.2
>>
>>
> 

Re: [PATCH] pci: check bus pointer before dereference

[P J P]

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

+-- On Tue, 15 Sep 2020, Philippe Mathieu-Daudé wrote --+
| > I think in normal this 'bus' will be not NULL. I have look at the link in 
| > the commit msg. I find it is another DMA to MMIO issue which we have 
| > discussed a lot but didn't come up with an satisfying solution.

  If 'bus' is unlikely to be NULL, should this be a regular non-CVE bug?
 
| As usual, question is how we got here.
| As Li said, it is another DMA to MMIO bug class.
| 
| lsi_execute_script
|  -> address_space_write
|     -> acpi_pcihp_eject_slot
|        -> bus_remove_child
| 
| So at this point the PCI device is still MMIO-mapped but eject from the 
| bus... ??? Then IRQ is triggered, which the device wants to propagate via 
| its PCI bus but it doesn't have any more and b00m.
| 
| If a device is hotpluggable, who is responsible to unmap its regions?

  Not sure, I guess I'll leave it for the upstream maintainers to device a 
better solution.

| Nack, this should be an abort().

===
diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index de0fae10ab..0ccb991410 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -253,6 +253,7 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int 
irq_num, int change)
     PCIBus *bus;
     for (;;) {
         bus = pci_get_bus(pci_dev);
+        assert(bus);
         irq_num = bus->map_irq(pci_dev, irq_num);
         if (bus->set_irq)
             break;
===

This should be okay for now?


Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[Peter Maydell]

On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
> ===
> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> index de0fae10ab..0ccb991410 100644
> --- a/hw/pci/pci.c
> +++ b/hw/pci/pci.c
> @@ -253,6 +253,7 @@ static void pci_change_irq_level(PCIDevice *pci_dev, int
> irq_num, int change)
>      PCIBus *bus;
>      for (;;) {
>          bus = pci_get_bus(pci_dev);
> +        assert(bus);
>          irq_num = bus->map_irq(pci_dev, irq_num);
>          if (bus->set_irq)
>              break;
> ===
>
> This should be okay for now?

Generally we don't bother to assert() that pointers that shouldn't be NULL
really are NULL immediately before dereferencing them, because the
dereference provides an equally easy-to-debug crash to the assert,
and so the assert doesn't provide anything extra.
assert()ing that a pointer is non-NULL is more useful if it is done in a
place that identifies the problem at an earlier and easier-to-debug point
in execution rather than at a later point which is distantly removed from
the place where the bogus pointer was introduced.

thanks
-- PMM

Re: [PATCH] pci: check bus pointer before dereference

[P J P]

  Hello,

+-- On Wed, 16 Sep 2020, Peter Maydell wrote --+
| On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
| > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
| > ==1183858==Hint: address points to the zero page.
| > #0 pci_change_irq_level hw/pci/pci.c:259
| > #1 pci_irq_handler hw/pci/pci.c:1445
| > #2 pci_set_irq hw/pci/pci.c:1463
| > #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
| > #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
| > #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
| > #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
| > #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
| > #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
...
| Generally we don't bother to assert() that pointers that shouldn't be NULL 
| really are NULL immediately before dereferencing them, because the 
| dereference provides an equally easy-to-debug crash to the assert, and so 
| the assert doesn't provide anything extra. assert()ing that a pointer is 
| non-NULL is more useful if it is done in a place that identifies the problem 
| at an earlier and easier-to-debug point in execution rather than at a later 
| point which is distantly removed from the place where the bogus pointer was 
| introduced.

* The NULL dereference above occurs because the 'pci_dev->qdev->parent_bus' 
  address gets overwritten (with 0x0) during scsi 'Memory Move' operation in

 ../hw/scsi/lsi53c895a.c
  #define LSI_BUF_SIZE 4096

lsi_mmio_write
 lsi_reg_writeb
  lsi_execute_script
   static void lsi_memcpy(LSIState *s, ... int count=12MB)
   {
      int n;
      uint8_t buf[LSI_BUF_SIZE];

      while (count) {
        n = (count > LSI_BUF_SIZE) ? LSI_BUF_SIZE : count;
        lsi_mem_read(s, src, buf, n);          <== read from DMA memory
        lsi_mem_write(s, dest, buf, n);        <== write to I/O memory
        src += n;
        dest += n;
        count -= n;
     }
   }
-> https://www.manualslib.com/manual/1407578/Lsi-Lsi53c895a.html?page=254#manual

* Above loop moves data between DMA memory to i/o address space.

* Going through the manual above, it seems 'Memory Move' can move upto 16MB of 
  data between memory spaces.

* I tried to see a suitable fix, but couldn't get one.

  - Limiting 'count' value does not seem right, as allowed value is upto 16MB.

  - Manual above talks about moving data via 'dma_buf'. But it doesn't seem to 
    be used here.

* During above loop, 'dest' address moves past its 'MemoryRegion mr' and 
  overwrites the adjacent 'mr' memory area, overwritting 'parent_bus' value.

Any thoughts/hints please...?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[P J P]

[+Paolo, +Fam Zheng - for scsi]

+-- On Mon, 28 Sep 2020, P J P wrote --+
| +-- On Wed, 16 Sep 2020, Peter Maydell wrote --+
| | On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
| | > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
| | > ==1183858==Hint: address points to the zero page.
| | > #0 pci_change_irq_level hw/pci/pci.c:259
| | > #1 pci_irq_handler hw/pci/pci.c:1445
| | > #2 pci_set_irq hw/pci/pci.c:1463
| | > #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
| | > #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
| | > #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
| | > #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
| | > #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
| | > #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
| ...
| | Generally we don't bother to assert() that pointers that shouldn't be NULL 
| | really are NULL immediately before dereferencing them, because the 
| | dereference provides an equally easy-to-debug crash to the assert, and so 
| | the assert doesn't provide anything extra. assert()ing that a pointer is 
| | non-NULL is more useful if it is done in a place that identifies the problem 
| | at an earlier and easier-to-debug point in execution rather than at a later 
| | point which is distantly removed from the place where the bogus pointer was 
| | introduced.
| 
| * The NULL dereference above occurs because the 'pci_dev->qdev->parent_bus' 
|   address gets overwritten (with 0x0) during scsi 'Memory Move' operation in
| 
|  ../hw/scsi/lsi53c895a.c
|   #define LSI_BUF_SIZE 4096
| 
| lsi_mmio_write
|  lsi_reg_writeb
|   lsi_execute_script
|    static void lsi_memcpy(LSIState *s, ... int count=12MB)
|    {
|       int n;
|       uint8_t buf[LSI_BUF_SIZE];
| 
|       while (count) {
|         n = (count > LSI_BUF_SIZE) ? LSI_BUF_SIZE : count;
|         lsi_mem_read(s, src, buf, n);          <== read from DMA memory
|         lsi_mem_write(s, dest, buf, n);        <== write to I/O memory
|         src += n;
|         dest += n;
|         count -= n;
|      }
|    }
| -> https://www.manualslib.com/manual/1407578/Lsi-Lsi53c895a.html?page=254#manual
| 
| * Above loop moves data between DMA memory to i/o address space.
| 
| * Going through the manual above, it seems 'Memory Move' can move upto 16MB of 
|   data between memory spaces.
| 
| * I tried to see a suitable fix, but couldn't get one.
| 
|   - Limiting 'count' value does not seem right, as allowed value is upto 16MB.
| 
|   - Manual above talks about moving data via 'dma_buf'. But it doesn't seem to 
|     be used here.
| 
| * During above loop, 'dest' address moves past its 'MemoryRegion mr' and 
|   overwrites the adjacent 'mr' memory area, overwritting 'parent_bus' value.
| 
| Any thoughts/hints please...?

@Paolo, @Fam...wdyt?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[Igor Mammedov]

On Wed, 30 Sep 2020 10:32:42 +0530 (IST)
P J P <ppandit@redhat.com> wrote:

> [+Paolo, +Fam Zheng - for scsi]
> 
> +-- On Mon, 28 Sep 2020, P J P wrote --+
> | +-- On Wed, 16 Sep 2020, Peter Maydell wrote --+
> | | On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
> | | > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
> | | > ==1183858==Hint: address points to the zero page.
> | | > #0 pci_change_irq_level hw/pci/pci.c:259
> | | > #1 pci_irq_handler hw/pci/pci.c:1445
> | | > #2 pci_set_irq hw/pci/pci.c:1463
> | | > #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
> | | > #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
> | | > #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
> | | > #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
> | | > #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
> | | > #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
> | ...
> | | Generally we don't bother to assert() that pointers that shouldn't be NULL 
> | | really are NULL immediately before dereferencing them, because the 
> | | dereference provides an equally easy-to-debug crash to the assert, and so 
> | | the assert doesn't provide anything extra. assert()ing that a pointer is 
> | | non-NULL is more useful if it is done in a place that identifies the problem 
> | | at an earlier and easier-to-debug point in execution rather than at a later 
> | | point which is distantly removed from the place where the bogus pointer was 
> | | introduced.
> | 
> | * The NULL dereference above occurs because the 'pci_dev->qdev->parent_bus' 
> |   address gets overwritten (with 0x0) during scsi 'Memory Move' operation in
> | 
> |  ../hw/scsi/lsi53c895a.c
> |   #define LSI_BUF_SIZE 4096
> | 
> | lsi_mmio_write
> |  lsi_reg_writeb
> |   lsi_execute_script
> |    static void lsi_memcpy(LSIState *s, ... int count=12MB)
> |    {
> |       int n;
> |       uint8_t buf[LSI_BUF_SIZE];
> | 
> |       while (count) {
> |         n = (count > LSI_BUF_SIZE) ? LSI_BUF_SIZE : count;
> |         lsi_mem_read(s, src, buf, n);          <== read from DMA memory
> |         lsi_mem_write(s, dest, buf, n);        <== write to I/O memory
> |         src += n;
> |         dest += n;
> |         count -= n;
> |      }
> |    }
> | -> https://www.manualslib.com/manual/1407578/Lsi-Lsi53c895a.html?page=254#manual
> | 
> | * Above loop moves data between DMA memory to i/o address space.
> | 
> | * Going through the manual above, it seems 'Memory Move' can move upto 16MB of 
> |   data between memory spaces.
> | 
> | * I tried to see a suitable fix, but couldn't get one.
> | 
> |   - Limiting 'count' value does not seem right, as allowed value is upto 16MB.
> | 
> |   - Manual above talks about moving data via 'dma_buf'. But it doesn't seem to 
> |     be used here.
> | 
> | * During above loop, 'dest' address moves past its 'MemoryRegion mr' and 
> |   overwrites the adjacent 'mr' memory area, overwritting 'parent_bus' value.
> | 
> | Any thoughts/hints please...?

'dest' is offset into MemoryRegion, so far I don't see how it could break into QEMU stack.
Do you have a simple reproducer?

> @Paolo, @Fam...wdyt?
> 
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[P J P]

+-- On Wed, 30 Sep 2020, Igor Mammedov wrote --+
| 'dest' is offset into MemoryRegion, so far I don't see how it could break 
| into QEMU stack. Do you have a simple reproducer?

Please see:
  -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[Michael S. Tsirkin]

On Wed, Sep 30, 2020 at 10:32:42AM +0530, P J P wrote:
> 
> [+Paolo, +Fam Zheng - for scsi]
> 
> +-- On Mon, 28 Sep 2020, P J P wrote --+
> | +-- On Wed, 16 Sep 2020, Peter Maydell wrote --+
> | | On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
> | | > -> https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
> | | > ==1183858==Hint: address points to the zero page.
> | | > #0 pci_change_irq_level hw/pci/pci.c:259
> | | > #1 pci_irq_handler hw/pci/pci.c:1445
> | | > #2 pci_set_irq hw/pci/pci.c:1463
> | | > #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
> | | > #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
> | | > #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
> | | > #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
> | | > #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
> | | > #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
> | ...
> | | Generally we don't bother to assert() that pointers that shouldn't be NULL 
> | | really are NULL immediately before dereferencing them, because the 
> | | dereference provides an equally easy-to-debug crash to the assert, and so 
> | | the assert doesn't provide anything extra. assert()ing that a pointer is 
> | | non-NULL is more useful if it is done in a place that identifies the problem 
> | | at an earlier and easier-to-debug point in execution rather than at a later 
> | | point which is distantly removed from the place where the bogus pointer was 
> | | introduced.
> | 
> | * The NULL dereference above occurs because the 'pci_dev->qdev->parent_bus' 
> |   address gets overwritten (with 0x0) during scsi 'Memory Move' operation in
> | 
> |  ../hw/scsi/lsi53c895a.c
> |   #define LSI_BUF_SIZE 4096
> | 
> | lsi_mmio_write
> |  lsi_reg_writeb
> |   lsi_execute_script
> |    static void lsi_memcpy(LSIState *s, ... int count=12MB)
> |    {
> |       int n;
> |       uint8_t buf[LSI_BUF_SIZE];
> | 
> |       while (count) {
> |         n = (count > LSI_BUF_SIZE) ? LSI_BUF_SIZE : count;
> |         lsi_mem_read(s, src, buf, n);          <== read from DMA memory
> |         lsi_mem_write(s, dest, buf, n);        <== write to I/O memory
> |         src += n;
> |         dest += n;
> |         count -= n;
> |      }
> |    }
> | -> https://www.manualslib.com/manual/1407578/Lsi-Lsi53c895a.html?page=254#manual
> | 
> | * Above loop moves data between DMA memory to i/o address space.
> | 
> | * Going through the manual above, it seems 'Memory Move' can move upto 16MB of 
> |   data between memory spaces.
> | 
> | * I tried to see a suitable fix, but couldn't get one.
> | 
> |   - Limiting 'count' value does not seem right, as allowed value is upto 16MB.
> | 
> |   - Manual above talks about moving data via 'dma_buf'. But it doesn't seem to 
> |     be used here.
> | 
> | * During above loop, 'dest' address moves past its 'MemoryRegion mr' and 
> |   overwrites the adjacent 'mr' memory area, overwritting 'parent_bus' value.
> | 
> | Any thoughts/hints please...?
> 
> @Paolo, @Fam...wdyt?
> 
> Thank you.

Guys are we going anywhere with this patch?

> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH] pci: check bus pointer before dereference

[Fam Zheng]

On Fri, 2020-10-30 at 05:01 -0400, Michael S. Tsirkin wrote:
> On Wed, Sep 30, 2020 at 10:32:42AM +0530, P J P wrote:
> > 
> > [+Paolo, +Fam Zheng - for scsi]
> > 
> > +-- On Mon, 28 Sep 2020, P J P wrote --+
> > > +-- On Wed, 16 Sep 2020, Peter Maydell wrote --+
> > > > On Wed, 16 Sep 2020 at 07:28, P J P <ppandit@redhat.com> wrote:
> > > > > -> 
> > > > > https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Flsi_nullptr1
> > > > > ==1183858==Hint: address points to the zero page.
> > > > > #0 pci_change_irq_level hw/pci/pci.c:259
> > > > > #1 pci_irq_handler hw/pci/pci.c:1445
> > > > > #2 pci_set_irq hw/pci/pci.c:1463
> > > > > #3 lsi_set_irq hw/scsi/lsi53c895a.c:488
> > > > > #4 lsi_update_irq hw/scsi/lsi53c895a.c:523
> > > > > #5 lsi_script_scsi_interrupt hw/scsi/lsi53c895a.c:554
> > > > > #6 lsi_execute_script hw/scsi/lsi53c895a.c:1149
> > > > > #7 lsi_reg_writeb hw/scsi/lsi53c895a.c:1984
> > > > > #8 lsi_io_write hw/scsi/lsi53c895a.c:2146
> > > 
> > > ...
> > > > Generally we don't bother to assert() that pointers that
> > > > shouldn't be NULL 
> > > > really are NULL immediately before dereferencing them, because
> > > > the 
> > > > dereference provides an equally easy-to-debug crash to the
> > > > assert, and so 
> > > > the assert doesn't provide anything extra. assert()ing that a
> > > > pointer is 
> > > > non-NULL is more useful if it is done in a place that
> > > > identifies the problem 
> > > > at an earlier and easier-to-debug point in execution rather
> > > > than at a later 
> > > > point which is distantly removed from the place where the bogus
> > > > pointer was 
> > > > introduced.
> > > 
> > > * The NULL dereference above occurs because the 'pci_dev->qdev-
> > > >parent_bus' 
> > >   address gets overwritten (with 0x0) during scsi 'Memory Move'
> > > operation in
> > > 
> > >  ../hw/scsi/lsi53c895a.c
> > >   #define LSI_BUF_SIZE 4096
> > > 
> > > lsi_mmio_write
> > >  lsi_reg_writeb
> > >   lsi_execute_script
> > >    static void lsi_memcpy(LSIState *s, ... int count=12MB)
> > >    {
> > >       int n;
> > >       uint8_t buf[LSI_BUF_SIZE];
> > > 
> > >       while (count) {
> > >         n = (count > LSI_BUF_SIZE) ? LSI_BUF_SIZE : count;
> > >         lsi_mem_read(s, src, buf, n);          <== read from DMA
> > > memory
> > >         lsi_mem_write(s, dest, buf, n);        <== write to I/O
> > > memory
> > >         src += n;
> > >         dest += n;
> > >         count -= n;
> > >      }
> > >    }
> > > -> 
> > > https://www.manualslib.com/manual/1407578/Lsi-Lsi53c895a.html?page=254#manual
> > > 
> > > * Above loop moves data between DMA memory to i/o address space.
> > > 
> > > * Going through the manual above, it seems 'Memory Move' can move
> > > upto 16MB of 
> > >   data between memory spaces.
> > > 
> > > * I tried to see a suitable fix, but couldn't get one.
> > > 
> > >   - Limiting 'count' value does not seem right, as allowed value
> > > is upto 16MB.
> > > 
> > >   - Manual above talks about moving data via 'dma_buf'. But it
> > > doesn't seem to 
> > >     be used here.
> > > 
> > > * During above loop, 'dest' address moves past its 'MemoryRegion
> > > mr' and 
> > >   overwrites the adjacent 'mr' memory area, overwritting
> > > 'parent_bus' value.

I agree with Igor, I don't understand how pci_dma_rw writing into the
next mr can cause the NULL pointer. parent_bus is in the QEMU heap, but
mr is backed by different ram areas, protected with boundary check.

Is there a backtrace at the corruption time?

Fam