All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
@ 2020-11-05 10:56 Mauro Matteo Cascella
  2020-11-05 11:21 ` Mauro Matteo Cascella
  2020-11-09  2:38 ` Jason Wang
  0 siblings, 2 replies; 7+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-05 10:56 UTC (permalink / raw)
  To: qemu-devel
  Cc: mcascell, dmitry.fleytman, gaoning.pgn, jasowang, lersek, 330cjfdn

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-05 10:56 [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance() Mauro Matteo Cascella
@ 2020-11-05 11:21 ` Mauro Matteo Cascella
  2020-11-09  2:38 ` Jason Wang
  1 sibling, 0 replies; 7+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-05 11:21 UTC (permalink / raw)
  To: QEMU Developers
  Cc: gaoning.pgn, Jason Wang, Dmitry Fleytman, lersek, 330cjfdn

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-05 10:56 [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance() Mauro Matteo Cascella
  2020-11-05 11:21 ` Mauro Matteo Cascella
@ 2020-11-09  2:38 ` Jason Wang
  2020-11-10  9:06   ` Mauro Matteo Cascella
  1 sibling, 1 reply; 7+ messages in thread
From: Jason Wang @ 2020-11-09  2:38 UTC (permalink / raw)
  To: Mauro Matteo Cascella, qemu-devel
  Cc: gaoning.pgn, 330cjfdn, dmitry.fleytman, lersek


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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-09  2:38 ` Jason Wang
@ 2020-11-10  9:06   ` Mauro Matteo Cascella
  2020-11-11  8:54     ` Jason Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-10  9:06 UTC (permalink / raw)
  To: Jason Wang
  Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-10  9:06   ` Mauro Matteo Cascella
@ 2020-11-11  8:54     ` Jason Wang
  2020-11-11 12:48       ` Jason Wang
  0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2020-11-11  8:54 UTC (permalink / raw)
  To: Mauro Matteo Cascella
  Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers


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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-11  8:54     ` Jason Wang
@ 2020-11-11 12:48       ` Jason Wang
  2020-11-12 10:20         ` Mauro Matteo Cascella
  0 siblings, 1 reply; 7+ messages in thread
From: Jason Wang @ 2020-11-11 12:48 UTC (permalink / raw)
  To: Mauro Matteo Cascella
  Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers


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





^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance()
  2020-11-11 12:48       ` Jason Wang
@ 2020-11-12 10:20         ` Mauro Matteo Cascella
  0 siblings, 0 replies; 7+ messages in thread
From: Mauro Matteo Cascella @ 2020-11-12 10:20 UTC (permalink / raw)
  To: Jason Wang
  Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-12 10:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-05 10:56 [PATCH] net/e1000e_core: make sure RDH never exceeds RDT in e1000e_ring_advance() Mauro Matteo Cascella
2020-11-05 11:21 ` Mauro Matteo Cascella
2020-11-09  2:38 ` Jason Wang
2020-11-10  9:06   ` Mauro Matteo Cascella
2020-11-11  8:54     ` Jason Wang
2020-11-11 12:48       ` Jason Wang
2020-11-12 10:20         ` Mauro Matteo Cascella

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.