Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

On Mon, Nov 9, 2020 at 3:38 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/5 下午6:56, Mauro Matteo Cascella wrote:
> > The e1000e_write_packet_to_guest() function iterates over a set of
> > receive descriptors by advancing rx descriptor head register (RDH) from
> > its initial value to rx descriptor tail register (RDT). The check in
> > e1000e_ring_empty() is responsible for detecting whether RDH has reached
> > RDT, terminating the loop if that's the case. Additional checks have
> > been added in the past to deal with bogus values submitted by the guest
> > to prevent possible infinite loop. This is done by "wrapping around" RDH
> > at some point and detecting whether it assumes the original value during
> > the loop.
> >
> > However, when e1000e is configured to use the packet split feature, RDH is
> > incremented by two instead of one, as the packet split descriptors are
> > 32 bytes while regular descriptors are 16 bytes. A malicious or buggy
> > guest may set RDT to an odd value and transmit only null RX descriptors.
> > This corner case would prevent RDH from ever matching RDT, leading to an
> > infinite loop. This patch adds a check in e1000e_ring_advance() to make
> > sure RDH does never exceed RDT.
> >
> > This issue was independently reported by Gaoning Pan (Zhejiang University)
> > and Cheolwoo Myung.
> >
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > Reported-by: Gaoning Pan <gaoning.pgn@antgroup.com>
> > Reported-by: Cheolwoo Myung <330cjfdn@gmail.com>
> > ---
> > References:
> > https://git.qemu.org/?p=qemu.git;a=commit;h=dd793a74882477ca38d49e191110c17dfe
> > https://git.qemu.org/?p=qemu.git;a=commit;h=4154c7e03fa55b4cf52509a83d50d6c09d743b7
> > http://www.intel.com/content/dam/doc/datasheet/82574l-gbe-controller-datasheet.pdf
> >
> >   hw/net/e1000e_core.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
> > index bcd186cac5..4c4d14b6ed 100644
> > --- a/hw/net/e1000e_core.c
> > +++ b/hw/net/e1000e_core.c
> > @@ -831,6 +831,10 @@ e1000e_ring_advance(E1000ECore *core, const 
> > E1000E_RingInfo *r, uint32_t count)
> >   {
> >       core->mac[r->dh] += count;
> >
> > +    if (core->mac[r->dh] > core->mac[r->dt]) {
> > +        core->mac[r->dh] = core->mac[r->dt];
> > +    }
> > +
> >       if (core->mac[r->dh] * E1000_RING_DESC_LEN >= core->mac[r->dlen]) {
> >           core->mac[r->dh] = 0;
> >       }

Hi Jason,

> A question here.
>
> When count > 1, is this correct to reset dh here?
>
> Thanks
>

My understanding is that wrapping at (or above) RDLEN is the correct
behavior regardless of count. I don't see why count > 1 should modify
this behavior. I'm not sure, though. Anyway, this patch fixes the
above reproducer, so I'm adding a Tested-by line here.

Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>

Thank you,
--
Mauro Matteo Cascella
Red Hat Product Security
PGP-Key ID: BB3410B0