| Message ID | 20201105105616.327593-1-mcascell@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance() | expand |
On Thu, Nov 5, 2020 at 11:56 AM Mauro Matteo Cascella <mcascell@redhat.com> 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; > } > -- > 2.26.2 > Here is a simple PoC to reproduce the aforementioned issue via qtest: cat << EOF | ./qemu-system-x86_64 -device e1000e -trace e1000e\* -display none -qtest stdio -accel qtest outl 0xcf8 0x80002010 outl 0xcfc 0xe0000000 outl 0xcf8 0x80002004 outw 0xcfc 0x7 write 0xe0003800 0x4 0x10001000 write 0xe0003808 0x4 0x00080000 write 0xe0000400 0x4 0xfa000400 write 0xe0002808 0x4 0x00080000 write 0xe0002818 0x4 0x7f000000 <== set RDT to 127 write 0xe0005008 0x4 0xfbffa3fa write 0xe0000100 0x4 0x5a840000 write 0x100018 0x2 0x6705 write 0x10001b 0x1 0x01 write 0xe0003818 0x4 0x04000000 EOF -- Mauro Matteo Cascella Red Hat Product Security PGP-Key ID: BB3410B0
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; > } A question here. When count > 1, is this correct to reset dh here? Thanks
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
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
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
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,
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; }
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(+)