On Thu, May 31, 2018 at 2:58 PM, sundeep subbaraya
<sundeep.l...@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!
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
