Re: [PATCH] scsi/lsi53c895a: restrict DMA engine to memory regions (CVE-2023-0330)

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 17/3/23 19:18, Karl Heubaum wrote:
> Did this CVE fix fall in the cracks during the QEMU 8.0 merge window?

The patch isn't reviewed, and apparently almost no active contributor
understand this device enough to be sure this security patch doesn't
break normal use. Fuzzed bugs are rarely trivial.

>> On Jan 16, 2023, at 2:42 PM, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>>
>> This prevents the well known DMA-MMIO reentrancy problem (upstream issue #556)
>> leading to memory corruption bugs like stack overflow or use-after-free.
>>
>> Fixes: CVE-2023-0330
>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> Reported-by: Zheyu Ma <zheyuma97@gmail.com>
>> ---
>> hw/scsi/lsi53c895a.c               | 14 +++++++++----
>> tests/qtest/fuzz-lsi53c895a-test.c | 32 ++++++++++++++++++++++++++++++
>> 2 files changed, 42 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
>> index af93557a9a..89c52594eb 100644
>> --- a/hw/scsi/lsi53c895a.c
>> +++ b/hw/scsi/lsi53c895a.c
>> @@ -446,22 +446,28 @@ static void lsi_reselect(LSIState *s, lsi_request *p);
>> static inline void lsi_mem_read(LSIState *s, dma_addr_t addr,
>>                                 void *buf, dma_addr_t len)
>> {
>> +    const MemTxAttrs attrs = { .memory = true };
>> +
>>      if (s->dmode & LSI_DMODE_SIOM) {
>> -        address_space_read(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED,
>> +        address_space_read(&s->pci_io_as, addr, attrs,
>>                             buf, len);
>>      } else {
>> -        pci_dma_read(PCI_DEVICE(s), addr, buf, len);
>> +        pci_dma_rw(PCI_DEVICE(s), addr, buf, len,
>> +                      DMA_DIRECTION_TO_DEVICE, attrs);
>>      }
>> }
>>
>> static inline void lsi_mem_write(LSIState *s, dma_addr_t addr,
>>                                  const void *buf, dma_addr_t len)
>> {
>> +    const MemTxAttrs attrs = { .memory = true };
>> +
>>      if (s->dmode & LSI_DMODE_DIOM) {
>> -        address_space_write(&s->pci_io_as, addr, MEMTXATTRS_UNSPECIFIED,
>> +        address_space_write(&s->pci_io_as, addr, attrs,
>>                              buf, len);
>>      } else {
>> -        pci_dma_write(PCI_DEVICE(s), addr, buf, len);
>> +        pci_dma_rw(PCI_DEVICE(s), addr, (void *) buf, len,
>> +                      DMA_DIRECTION_FROM_DEVICE, attrs);
>>      }
>> }
>>
>> diff --git a/tests/qtest/fuzz-lsi53c895a-test.c b/tests/qtest/fuzz-lsi53c895a-test.c
>> index 392a7ae7ed..35c02e89f3 100644
>> --- a/tests/qtest/fuzz-lsi53c895a-test.c
>> +++ b/tests/qtest/fuzz-lsi53c895a-test.c
>> @@ -8,6 +8,35 @@
>> #include "qemu/osdep.h"
>> #include "libqtest.h"
>>
>> +/*
>> + * This used to trigger a DMA reentrancy issue
>> + * leading to memory corruption bugs like stack
>> + * overflow or use-after-free
>> + */
>> +static void test_lsi_dma_reentrancy(void)
>> +{
>> +    QTestState *s;
>> +
>> +    s = qtest_init("-M q35 -m 512M -nodefaults "
>> +                   "-blockdev driver=null-co,node-name=null0 "
>> +                   "-device lsi53c810 -device scsi-cd,drive=null0");
>> +
>> +    qtest_outl(s, 0xcf8, 0x80000804); /* PCI Command Register */
>> +    qtest_outw(s, 0xcfc, 0x7);        /* Enables accesses */
>> +    qtest_outl(s, 0xcf8, 0x80000814); /* Memory Bar 1 */
>> +    qtest_outl(s, 0xcfc, 0xff100000); /* Set MMIO Address*/
>> +    qtest_outl(s, 0xcf8, 0x80000818); /* Memory Bar 2 */
>> +    qtest_outl(s, 0xcfc, 0xff000000); /* Set RAM Address*/
>> +    qtest_writel(s, 0xff000000, 0xc0000024);
>> +    qtest_writel(s, 0xff000114, 0x00000080);
>> +    qtest_writel(s, 0xff00012c, 0xff000000);
>> +    qtest_writel(s, 0xff000004, 0xff000114);
>> +    qtest_writel(s, 0xff000008, 0xff100014);
>> +    qtest_writel(s, 0xff10002f, 0x000000ff);
>> +
>> +    qtest_quit(s);
>> +}
>> +
>> /*
>>   * This used to trigger a UAF in lsi_do_msgout()
>>   * https://gitlab.com/qemu-project/qemu/-/issues/972
>> @@ -120,5 +149,8 @@ int main(int argc, char **argv)
>>      qtest_add_func("fuzz/lsi53c895a/lsi_do_msgout_cancel_req",
>>                     test_lsi_do_msgout_cancel_req);
>>
>> +    qtest_add_func("fuzz/lsi53c895a/lsi_dma_reentrancy",
>> +                   test_lsi_dma_reentrancy);
>> +
>>      return g_test_run();
>> }
>> -- 
>> 2.39.0
>>
>>
>