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

[Julia Suvorova <jusual@mail.ru>]

On 01.06.2018 13:44, Stefan Hajnoczi wrote:
> On Fri, Jun 1, 2018 at 11:41 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
>> <sundeep.lkml@gmail.com> wrote:
>>> On Wed, May 30, 2018 at 3:33 AM, Julia Suvorova via Qemu-devel
>>> <qemu-devel@nongnu.org> 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.

Even if I change the reg array to work with the R_* macros, it will
still be inefficient, since registers are not sequentially located.
I can define the registers in the Nrf51UART structure separately, but
this will increase the code size. Is there a proper way?

Best regards, Julia Suvorova.