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

[Mauro Matteo Cascella]

On Wed, Nov 11, 2020 at 1:48 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/11 下午4:54, Jason Wang wrote:
> >
> > On 2020/11/10 下午5:06, Mauro Matteo Cascella wrote:
> >> 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
> >>
> >
> > Right.
> >
> > Applied.
> >
> > Thanks
>
>
> I had to drop this since it breaks e1000e PXE test.
>
> Thanks
>

By debugging the failing qtest (/x86_64/pxe/ipv4/q35/e1000e) I noticed
several cases where RDH > RDT in e1000e_ring_advance(). Given the
RX/TX descriptor ring structure, I guess this is a possible scenario
when the tail pointer wraps back to base when maximum descriptors have
been processed. I will send a new version to only cover cases where
RDH < RDT and the increment would exceed RDT. This should be enough to
fix the infinite loop issue while making the e1000e PXE test pass.

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