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