Re: [Qemu-devel] [PATCH 1/2] Ignore RX tail kicks when RX disabled.

[Dmitry Fleytman <dmitry@daynix.com>]

Hello, Stefan

The problem occurs between steps 2 and 3.
Let's say packet arrives after step 2 is done by driver.

Head and tail are 0 because of step 1

Check_rxov is 0 because of two reasons:
    1. On startup it is 0 by default
    2. It is zeroed by setting ring tail to 0 on first step

Then first check ( __if (!(s->mac_reg[RCTL] & E1000_RCTL_EN))__ )
passes because RX enabled on step 2.
e1000_has_rxbufs() returs true because it treats equal head and tail
as fully filled ring when check_rxov is 0:

    static bool e1000_has_rxbufs(E1000State *s, size_t total_size)
    {
        [...]

        if (total_size <= s->rxbuf_size) {
            return s->mac_reg[RDH] != s->mac_reg[RDT] || !s->check_rxov;

        [...]

        } else if (s->mac_reg[RDH] > s->mac_reg[RDT] || !s->check_rxov) {

       [...]
    }

So QEMU reads uninitialized descriptor and tries to perform "DMA" to
arbitrary address from descriptor.
Depending on address value it corrupts guest memory or abort()'s here:

void *qemu_get_ram_ptr(ram_addr_t addr)
{
    [...]

    fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
    abort();

    [...]
}

Thanks for review,
Dmitry.

On Thu, Oct 18, 2012 at 9:31 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> On Wed, Oct 17, 2012 at 08:31:46PM +0200, Dmitry Fleytman wrote:
> > Device RX initization from driver's side consists of following steps:
> >   1. Initialize head and tail of RX ring to 0
> >   2. Enable Rx (set bit in RCTL register)
> >   3. Allocate buffers, fill descriptors
> >   4. Write ring tail
> >
> > Forth operation signals hardware that RX buffers available
> > and it may start packets indication.
> >
> > Current implementation treats first operation (write 0 to ring tail)
> > as signal of buffers availability and starts data transfers as soon
> > as RX enable indicaton arrives.
> >
> > This is not correct because there is a chance that ring is still
> > empty (third action not performed yet) and then memory corruption
> > occures.
>
> The existing code tries to prevent this:
>
> e1000_receive(NetClientState *nc, const uint8_t *buf, size_t size)
> {
>     [...]
>
>     if (!(s->mac_reg[RCTL] & E1000_RCTL_EN))
>         return -1;
>
>     [...]
>     total_size = size + fcs_len(s);
>     if (!e1000_has_rxbufs(s, total_size)) {
>             set_ics(s, 0, E1000_ICS_RXO);
>             return -1;
>     }
>
> Why are these checks not enough?
>
> Which memory gets corrupted?
>
> Stefan




--
Dmitry Fleytman
Technology Expert and Consultant,

Daynix Computing Ltd.

Cell: +972-54-2819481

Skype: dmitry.fleytman