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
