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

[Stefan Hajnoczi]

On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <address@hidden> wrote:
> 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!

Sorry, I was wrong.  The array is large enough after all.  It's just
an inefficiency, but still worth fixing.  Similar issues could lead to
out-of-bound accesses.

Stefan