Re: [PATCH v5 1/6] hw/hppa/dino.c: Improve emulation of Dino PCI chip

[Philippe Mathieu-Daudé]

On 2/13/20 12:37 AM, Philippe Mathieu-Daudé wrote:
> Hi Sven, Helge.
> 
> On 12/20/19 10:15 PM, Sven Schnelle wrote:
>> From: Helge Deller <address@hidden>
>>
>> The tests of the dino chip with the Online-diagnostics CD
>> ("ODE DINOTEST") now succeeds.
>> Additionally add some qemu trace events.
>>
>> Signed-off-by: Helge Deller <address@hidden>
>> Signed-off-by: Sven Schnelle <address@hidden>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
>> ---
>>   MAINTAINERS          |  2 +-
>>   hw/hppa/dino.c       | 97 +++++++++++++++++++++++++++++++++++++-------
>>   hw/hppa/trace-events |  5 +++
>>   3 files changed, 89 insertions(+), 15 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 387879aebc..e333bc67a4 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -876,7 +876,7 @@ F: hw/*/etraxfs_*.c
>>     HP-PARISC Machines
>>   ------------------
>> -Dino
>> +HP B160L
>>   M: Richard Henderson <address@hidden>
>>   R: Helge Deller <address@hidden>
>>   S: Odd Fixes
>> diff --git a/hw/hppa/dino.c b/hw/hppa/dino.c
>> index ab6969b45f..9797a7f0d9 100644
>> --- a/hw/hppa/dino.c
>> +++ b/hw/hppa/dino.c
>> @@ -1,7 +1,7 @@
>>   /*
>> - * HP-PARISC Dino PCI chipset emulation.
>> + * HP-PARISC Dino PCI chipset emulation, as in B160L and similiar
>> machines
>>    *
>> - * (C) 2017 by Helge Deller <address@hidden>
>> + * (C) 2017-2019 by Helge Deller <address@hidden>
>>    *
>>    * This work is licensed under the GNU GPL license version 2 or later.
>>    *
>> @@ -21,6 +21,7 @@
>>   #include "migration/vmstate.h"
>>   #include "hppa_sys.h"
>>   #include "exec/address-spaces.h"
>> +#include "trace.h"
>>       #define TYPE_DINO_PCI_HOST_BRIDGE "dino-pcihost"
>> @@ -82,11 +83,28 @@
>>   #define DINO_PCI_HOST_BRIDGE(obj) \
>>       OBJECT_CHECK(DinoState, (obj), TYPE_DINO_PCI_HOST_BRIDGE)
>>   +#define DINO800_REGS ((DINO_TLTIM - DINO_GMASK) / 4)
> 
> Coverity noticed (CID 1419394) this should be '(1 + (DINO_TLTIM -
> DINO_GMASK) / 4)' (13 registers).
> 
>> +static const uint32_t reg800_keep_bits[DINO800_REGS] = {
>> +            MAKE_64BIT_MASK(0, 1),
>> +            MAKE_64BIT_MASK(0, 7),
>> +            MAKE_64BIT_MASK(0, 7),
>> +            MAKE_64BIT_MASK(0, 8),
>> +            MAKE_64BIT_MASK(0, 7),
>> +            MAKE_64BIT_MASK(0, 9),
>> +            MAKE_64BIT_MASK(0, 32),
> 
> Looking at the datasheet pp. 30, this register is 'Undefined'.
> We should report GUEST_ERROR if it is accessed.
> 
>> +            MAKE_64BIT_MASK(0, 8),
>> +            MAKE_64BIT_MASK(0, 30),
>> +            MAKE_64BIT_MASK(0, 25),
> 
> Still looking at the datasheet (pp. 37) PCIROR is 24-bit (not 25).
> 
>> +            MAKE_64BIT_MASK(0, 22),
> 
> Here you are missing register 0x82c...
> 
>> +            MAKE_64BIT_MASK(0, 9),
>> +};
>> +
> 
> Altogether:
> 
> static const uint32_t reg800_keep_bits[DINO800_REGS] = {
>     MAKE_64BIT_MASK(0, 1),  /* GMASK */
>     MAKE_64BIT_MASK(0, 7),  /* PAMR */
>     MAKE_64BIT_MASK(0, 7),  /* PAPR */
>     MAKE_64BIT_MASK(0, 8),  /* DAMODE */
>     MAKE_64BIT_MASK(0, 7),  /* PCICMD */
>     MAKE_64BIT_MASK(0, 9),  /* PCISTS */
>     MAKE_64BIT_MASK(0, 0),  /* Undefined */
>     MAKE_64BIT_MASK(0, 8),  /* MLTIM */
>     MAKE_64BIT_MASK(0, 30), /* BRDG_FEAT */
>     MAKE_64BIT_MASK(0, 24), /* PCIROR */
>     MAKE_64BIT_MASK(0, 22), /* PCIWOR */
>     MAKE_64BIT_MASK(0, 0),  /* Undocumented */
>     MAKE_64BIT_MASK(0, 9),  /* TLTIM */
> };
> 
>>   typedef struct DinoState {
>>       PCIHostState parent_obj;
>>         /* PCI_CONFIG_ADDR is parent_obj.config_reg, via
>> pci_host_conf_be_ops,
>>          so that we can map PCI_CONFIG_DATA to pci_host_data_be_ops.  */
>> +    uint32_t config_reg_dino; /* keep original copy, including 2
>> lowest bits */
>>         uint32_t iar0;
>>       uint32_t iar1;
>> @@ -94,8 +112,12 @@ typedef struct DinoState {
>>       uint32_t ipr;
>>       uint32_t icr;
>>       uint32_t ilr;
>> +    uint32_t io_fbb_en;
>>       uint32_t io_addr_en;
>>       uint32_t io_control;
>> +    uint32_t toc_addr;
>> +
>> +    uint32_t reg800[DINO800_REGS];
>>         MemoryRegion this_mem;
>>       MemoryRegion pci_mem;
>> @@ -106,8 +128,6 @@ typedef struct DinoState {
>>       MemoryRegion bm_ram_alias;
>>       MemoryRegion bm_pci_alias;
>>       MemoryRegion bm_cpu_alias;
>> -
>> -    MemoryRegion cpu0_eir_mem;
>>   } DinoState;
>>     /*
>> @@ -122,6 +142,8 @@ static void gsc_to_pci_forwarding(DinoState *s)
>>       tmp = extract32(s->io_control, 7, 2);
>>       enabled = (tmp == 0x01);
>>       io_addr_en = s->io_addr_en;
>> +    /* Mask out first (=firmware) and last (=Dino) areas. */
>> +    io_addr_en &= ~(BIT(31) | BIT(0));
>>         memory_region_transaction_begin();
>>       for (i = 1; i < 31; i++) {
>> @@ -142,6 +164,8 @@ static bool dino_chip_mem_valid(void *opaque,
>> hwaddr addr,
>>                                   unsigned size, bool is_write,
>>                                   MemTxAttrs attrs)
>>   {
>> +    bool ret = false;
>> +
>>       switch (addr) {
>>       case DINO_IAR0:
>>       case DINO_IAR1:
>> @@ -152,16 +176,22 @@ static bool dino_chip_mem_valid(void *opaque,
>> hwaddr addr,
>>       case DINO_ICR:
>>       case DINO_ILR:
>>       case DINO_IO_CONTROL:
>> +    case DINO_IO_FBB_EN:
>>       case DINO_IO_ADDR_EN:
>>       case DINO_PCI_IO_DATA:
>> -        return true;
>> +    case DINO_TOC_ADDR:
>> +    case DINO_GMASK ... DINO_TLTIM:
>> +        ret = true;
>> +        break;
>>       case DINO_PCI_IO_DATA + 2:
>> -        return size <= 2;
>> +        ret = (size <= 2);
>> +        break;
>>       case DINO_PCI_IO_DATA + 1:
>>       case DINO_PCI_IO_DATA + 3:
>> -        return size == 1;
>> +        ret = (size == 1);
>>       }
>> -    return false;
>> +    trace_dino_chip_mem_valid(addr, ret);
>> +    return ret;
>>   }
>>     static MemTxResult dino_chip_read_with_attrs(void *opaque, hwaddr
>> addr,
>> @@ -194,6 +224,9 @@ static MemTxResult dino_chip_read_with_attrs(void
>> *opaque, hwaddr addr,
>>           }
>>           break;
>>   +    case DINO_IO_FBB_EN:
>> +        val = s->io_fbb_en;
>> +        break;
>>       case DINO_IO_ADDR_EN:
>>           val = s->io_addr_en;
>>           break;
>> @@ -227,12 +260,28 @@ static MemTxResult
>> dino_chip_read_with_attrs(void *opaque, hwaddr addr,
>>       case DINO_IRR1:
>>           val = s->ilr & s->imr & s->icr;
>>           break;
>> +    case DINO_TOC_ADDR:
>> +        val = s->toc_addr;
>> +        break;
>> +    case DINO_GMASK ... DINO_TLTIM:
>> +        val = s->reg800[(addr - DINO_GMASK) / 4];
>> +        if (addr == DINO_PAMR) {
>> +            val &= ~0x01;  /* LSB is hardwired to 0 */
>> +        }
>> +        if (addr == DINO_MLTIM) {
>> +            val &= ~0x07;  /* 3 LSB are hardwired to 0 */
>> +        }
>> +        if (addr == DINO_BRDG_FEAT) {
>> +            val &= ~(0x10710E0ul | 8); /* bits 5-7, 24 & 15 reserved */
>> +        }
>> +        break;
>>         default:
>>           /* Controlled by dino_chip_mem_valid above.  */
>>           g_assert_not_reached();
>>       }
>>   +    trace_dino_chip_read(addr, val);
>>       *data = val;
>>       return ret;
>>   }
>> @@ -245,6 +294,9 @@ static MemTxResult dino_chip_write_with_attrs(void
>> *opaque, hwaddr addr,
>>       AddressSpace *io;
>>       MemTxResult ret;
>>       uint16_t ioaddr;
>> +    int i;
>> +
>> +    trace_dino_chip_write(addr, val);
>>         switch (addr) {
>>       case DINO_IO_DATA ... DINO_PCI_IO_DATA + 3:
>> @@ -266,9 +318,11 @@ static MemTxResult
>> dino_chip_write_with_attrs(void *opaque, hwaddr addr,
>>           }
>>           return ret;
>>   +    case DINO_IO_FBB_EN:
>> +        s->io_fbb_en = val & 0x03;
>> +        break;
>>       case DINO_IO_ADDR_EN:
>> -        /* Never allow first (=firmware) and last (=Dino) areas.  */
>> -        s->io_addr_en = val & 0x7ffffffe;
>> +        s->io_addr_en = val;
>>           gsc_to_pci_forwarding(s);
>>           break;
>>       case DINO_IO_CONTROL:
>> @@ -292,6 +346,10 @@ static MemTxResult
>> dino_chip_write_with_attrs(void *opaque, hwaddr addr,
>>           /* Any write to IPR clears the register.  */
>>           s->ipr = 0;
>>           break;
>> +    case DINO_TOC_ADDR:
>> +        /* IO_COMMAND of CPU with client_id bits */
>> +        s->toc_addr = 0xFFFA0030 | (val & 0x1e000);
>> +        break;
>>         case DINO_ILR:
>>       case DINO_IRR0:
>> @@ -299,6 +357,12 @@ static MemTxResult
>> dino_chip_write_with_attrs(void *opaque, hwaddr addr,
>>           /* These registers are read-only.  */
>>           break;
>>   +    case DINO_GMASK ... DINO_TLTIM:
>> +        i = (addr - DINO_GMASK) / 4;
>> +        val &= reg800_keep_bits[i];
> 
> Due to the missing register, Coverity reports:
> 
>>>>     CID 1419394:  Memory - illegal accesses  (OVERRUN)
>>>>     Overrunning array "reg800_keep_bits" of 12 4-byte elements at
> element index 12 (byte offset 48) using index "i" (which evaluates to 12).
> 
>> +        s->reg800[i] = val;
>> +        break; > +
>>       default:
>>           /* Controlled by dino_chip_mem_valid above.  */
>>           g_assert_not_reached();

Easy reproducer:

$ echo writeb 0xfff80830 0x69 \
  | hppa-softmmu/qemu-system-hppa -S -accel qtest -qtest stdio -display
none
[I 1581634452.654113] OPENED
[R +4.105415] writeb 0xfff80830 0x69
qemu/hw/hppa/dino.c:362:16: runtime error: index 12 out of bounds for
type 'const uint32_t [12]'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior
qemu/hw/hppa/dino.c:362:16 in
=================================================================
==29607==ERROR: AddressSanitizer: global-buffer-overflow on address
0x5577dae32f30 at pc 0x5577d93f2463 bp 0x7ffd97ea11b0 sp 0x7ffd97ea11a8
READ of size 4 at 0x5577dae32f30 thread T0
    #0 0x5577d93f2462 in dino_chip_write_with_attrs
qemu/hw/hppa/dino.c:362:16
    #1 0x5577d9025664 in memory_region_write_with_attrs_accessor
qemu/memory.c:503:12
    #2 0x5577d9024920 in access_with_adjusted_size qemu/memory.c:539:18
    #3 0x5577d9023608 in memory_region_dispatch_write qemu/memory.c:1482:13
    #4 0x5577d8e3177a in flatview_write_continue qemu/exec.c:3166:23
    #5 0x5577d8e20357 in flatview_write qemu/exec.c:3206:14
    #6 0x5577d8e1fef4 in address_space_write qemu/exec.c:3296:18
    #7 0x5577d8e20693 in address_space_rw qemu/exec.c:3306:16
    #8 0x5577d9011595 in qtest_process_command qemu/qtest.c:432:13
    #9 0x5577d900d19f in qtest_process_inbuf qemu/qtest.c:705:9
    #10 0x5577d900ca22 in qtest_read qemu/qtest.c:717:5
    #11 0x5577da8c4254 in qemu_chr_be_write_impl qemu/chardev/char.c:183:9
    #12 0x5577da8c430c in qemu_chr_be_write qemu/chardev/char.c:195:9
    #13 0x5577da8cf587 in fd_chr_read qemu/chardev/char-fd.c:68:9
    #14 0x5577da9836cd in qio_channel_fd_source_dispatch
qemu/io/channel-watch.c:84:12
    #15 0x7faf44509ecc in g_main_context_dispatch
(/lib64/libglib-2.0.so.0+0x4fecc)
    #16 0x5577dab75f96 in glib_pollfds_poll qemu/util/main-loop.c:219:9
    #17 0x5577dab74797 in os_host_main_loop_wait qemu/util/main-loop.c:242:5
    #18 0x5577dab7435a in main_loop_wait qemu/util/main-loop.c:518:11
    #19 0x5577d9514eb3 in main_loop qemu/vl.c:1682:9
    #20 0x5577d950699d in main qemu/vl.c:4450:5
    #21 0x7faf41a87f42 in __libc_start_main (/lib64/libc.so.6+0x23f42)
    #22 0x5577d8cd4d4d in _start
(qemu/build/sanitizer/hppa-softmmu/qemu-system-hppa+0x1256d4d)

0x5577dae32f30 is located 0 bytes to the right of global variable
'reg800_keep_bits' defined in 'qemu/hw/hppa/dino.c:87:23'
(0x5577dae32f00) of size 48
SUMMARY: AddressSanitizer: global-buffer-overflow
qemu/hw/hppa/dino.c:362:16 in dino_chip_write_with_attrs
Shadow bytes around the buggy address:
  0x0aaf7b5be590: 00 f9 f9 f9 f9 f9 f9 f9 00 02 f9 f9 f9 f9 f9 f9
  0x0aaf7b5be5a0: 07 f9 f9 f9 f9 f9 f9 f9 07 f9 f9 f9 f9 f9 f9 f9
  0x0aaf7b5be5b0: 07 f9 f9 f9 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0aaf7b5be5c0: 00 00 00 02 f9 f9 f9 f9 00 00 00 00 00 00 00 00
  0x0aaf7b5be5d0: 00 00 00 00 00 00 00 00 00 00 00 03 f9 f9 f9 f9
=>0x0aaf7b5be5e0: 00 00 00 00 00 00[f9]f9 f9 f9 f9 f9 00 00 00 00
  0x0aaf7b5be5f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aaf7b5be600: 00 00 01 f9 f9 f9 f9 f9 00 00 00 00 07 f9 f9 f9
  0x0aaf7b5be610: f9 f9 f9 f9 00 00 00 00 00 00 00 00 00 00 00 00
  0x0aaf7b5be620: 00 00 00 05 f9 f9 f9 f9 00 00 00 00 07 f9 f9 f9
  0x0aaf7b5be630: f9 f9 f9 f9 00 00 f9 f9 f9 f9 f9 f9 07 f9 f9 f9
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==29607==ABORTING