On Wed, Nov 11, 2020 at 1:48 PM Jason Wang <jasow...@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 <jasow...@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 <mcasc...@redhat.com> > >>>> Reported-by: Gaoning Pan <gaoning....@antgroup.com> > >>>> Reported-by: Cheolwoo Myung <330cj...@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 <mcasc...@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
