Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
<address@hidden> wrote:
> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
> <address@hidden> wrote:
>> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
>> +{
>> +    Nrf51UART *s = NRF51_UART(opaque);
>> +    uint64_t r;
>> +
>> +    switch (addr) {
>> +    case A_RXD:
>> +        r = s->rx_fifo[s->rx_fifo_pos];
>> +        if (s->rx_fifo_len > 0) {
>> +            s->rx_fifo_pos = (s->rx_fifo_pos + 1) % UART_FIFO_LENGTH;
>> +            s->rx_fifo_len--;
>> +            qemu_chr_fe_accept_input(&s->chr);
>> +        }
>> +        break;
>> +
>> +    case A_INTENSET:
>> +    case A_INTENCLR:
>> +    case A_INTEN:
>> +        r = s->reg[A_INTEN];
>> +        break;
>> +    default:
>> +        r = s->reg[addr];
>
> You can use R_* macros for registers and access regs[ ] with addr/4 as index.
> It is better than using big regs[ ] array out of which most of
> locations go unused.

Good point.  The bug is more severe than an inefficiency.
s->reg[addr] allows out-of-bounds accesses.  This is a security bug.

The memory region is 0x1000 *bytes* long, but the array has 0x1000
32-bit *elements*.  A read from address 0xfffc results in a memory
load from s->reg + 0xfffc * sizeof(s->reg[0]).  That's beyond the end
of the array!

s->reg[A_*] should be changed to s->reg[R_*].  s->reg[addr] needs to
be s->reg[addr / sizeof(s->reg[0])].

It may be worth adding a warning to scripts/checkpatch.pl for
array[A_*] so this bug is reported automatically in the future.

Stefan