[PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

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 not exceed RDT in a single incremental step, adjusting the count
value accordingly.

This issue was independently reported by Gaoning Pan (Zhejiang University)
and Cheolwoo Myung.

Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
Tested-by: Mauro Matteo Cascella <mcascell@redhat.com>
Reported-by: Gaoning Pan <gaoning.pgn@antgroup.com>
Reported-by: Cheolwoo Myung <330cjfdn@gmail.com>
---
Changes since previous version:
Take the initial values of RDH/RDT into account. Address the case where RDH is
less than RDT and (RDH + count) would exceed RDT.

Patch v1:
https://lists.nongnu.org/archive/html/qemu-devel/2020-11/msg01492.html

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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/hw/net/e1000e_core.c b/hw/net/e1000e_core.c
index bcfd46696f..324cc14ffb 100644
--- a/hw/net/e1000e_core.c
+++ b/hw/net/e1000e_core.c
@@ -829,6 +829,11 @@ e1000e_ring_head_descr(E1000ECore *core, const E1000E_RingInfo *r)
 static inline void
 e1000e_ring_advance(E1000ECore *core, const E1000E_RingInfo *r, uint32_t count)
 {
+    if (count > 1 && core->mac[r->dh] < core->mac[r->dt] &&
+        core->mac[r->dh] + count > core->mac[r->dt]) {
+        count = core->mac[r->dt] - core->mac[r->dh];
+    }
+
     core->mac[r->dh] += count;
 
     if (core->mac[r->dh] * E1000_RING_DESC_LEN >= core->mac[r->dlen]) {
-- 
2.28.0

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang]

On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> value accordingly.


Can this patch solve this issue in another way?

https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/

Thanks

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> > value accordingly.
>
>
> Can this patch solve this issue in another way?
>
> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>
> Thanks
>

Yes, it does work nicely. Still, I think this patch is useful to avoid
possible inconsistent state in e1000e_ring_advance() when count > 1.

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

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang]

On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>> value accordingly.
>>
>> Can this patch solve this issue in another way?
>>
>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>
>> Thanks
>>
> Yes, it does work nicely. Still, I think this patch is useful to avoid
> possible inconsistent state in e1000e_ring_advance() when count > 1.


So if RDT is odd, it looks to me the following codes in 
e1000e_write_packet_to_guest() needs to be fixed as well.


         base = e1000e_ring_head_descr(core, rxi);

         pci_dma_read(d, base, &desc, core->rx_desc_len);

Otherwise e1000e may try to read out of descriptor ring.

Thanks


>
> Thank you,

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> > On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>> value accordingly.
> >>
> >> Can this patch solve this issue in another way?
> >>
> >> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>
> >> Thanks
> >>
> > Yes, it does work nicely. Still, I think this patch is useful to avoid
> > possible inconsistent state in e1000e_ring_advance() when count > 1.
>
>
> So if RDT is odd, it looks to me the following codes in
> e1000e_write_packet_to_guest() needs to be fixed as well.
>
>
>          base = e1000e_ring_head_descr(core, rxi);
>
>          pci_dma_read(d, base, &desc, core->rx_desc_len);
>
> Otherwise e1000e may try to read out of descriptor ring.

Sorry, I'm not sure I understand what you mean. Isn't the base address
computed from RDH? How can e1000e read out of the descriptor ring if
RDT is odd?

>
> Thanks


On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> > On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>> value accordingly.
> >>
> >> Can this patch solve this issue in another way?
> >>
> >> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>
> >> Thanks
> >>
> > Yes, it does work nicely. Still, I think this patch is useful to avoid
> > possible inconsistent state in e1000e_ring_advance() when count > 1.
>
>
> So if RDT is odd, it looks to me the following codes in
> e1000e_write_packet_to_guest() needs to be fixed as well.
>
>
>          base = e1000e_ring_head_descr(core, rxi);
>
>          pci_dma_read(d, base, &desc, core->rx_desc_len);
>
> Otherwise e1000e may try to read out of descriptor ring.
>
> Thanks
>
>
> >
> > Thank you,
>


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

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang]

On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>> value accordingly.
>>>> Can this patch solve this issue in another way?
>>>>
>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>
>>>> Thanks
>>>>
>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>
>> So if RDT is odd, it looks to me the following codes in
>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>
>>
>>           base = e1000e_ring_head_descr(core, rxi);
>>
>>           pci_dma_read(d, base, &desc, core->rx_desc_len);
>>
>> Otherwise e1000e may try to read out of descriptor ring.
> Sorry, I'm not sure I understand what you mean. Isn't the base address
> computed from RDH? How can e1000e read out of the descriptor ring if
> RDT is odd?
>
>> Thanks
>
> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>> value accordingly.
>>>> Can this patch solve this issue in another way?
>>>>
>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>
>>>> Thanks
>>>>
>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>
>> So if RDT is odd, it looks to me the following codes in
>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>
>>
>>           base = e1000e_ring_head_descr(core, rxi);
>>
>>           pci_dma_read(d, base, &desc, core->rx_desc_len);
>>
>> Otherwise e1000e may try to read out of descriptor ring.
>>
>> Thanks


Sorry, I meant RDH actually, when packet split descriptor is used, it 
doesn't check whether DH exceeds DLEN?

Thanks


>>
>>
>>> Thank you,
>

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
> > On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> >>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>>>> value accordingly.
> >>>> Can this patch solve this issue in another way?
> >>>>
> >>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>>>
> >>>> Thanks
> >>>>
> >>> Yes, it does work nicely. Still, I think this patch is useful to avoid
> >>> possible inconsistent state in e1000e_ring_advance() when count > 1.
> >>
> >> So if RDT is odd, it looks to me the following codes in
> >> e1000e_write_packet_to_guest() needs to be fixed as well.
> >>
> >>
> >>           base = e1000e_ring_head_descr(core, rxi);
> >>
> >>           pci_dma_read(d, base, &desc, core->rx_desc_len);
> >>
> >> Otherwise e1000e may try to read out of descriptor ring.
> > Sorry, I'm not sure I understand what you mean. Isn't the base address
> > computed from RDH? How can e1000e read out of the descriptor ring if
> > RDT is odd?
> >
> >> Thanks
> >
> > On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> >>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>>>> value accordingly.
> >>>> Can this patch solve this issue in another way?
> >>>>
> >>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>>>
> >>>> Thanks
> >>>>
> >>> Yes, it does work nicely. Still, I think this patch is useful to avoid
> >>> possible inconsistent state in e1000e_ring_advance() when count > 1.
> >>
> >> So if RDT is odd, it looks to me the following codes in
> >> e1000e_write_packet_to_guest() needs to be fixed as well.
> >>
> >>
> >>           base = e1000e_ring_head_descr(core, rxi);
> >>
> >>           pci_dma_read(d, base, &desc, core->rx_desc_len);
> >>
> >> Otherwise e1000e may try to read out of descriptor ring.
> >>
> >> Thanks
>
>
> Sorry, I meant RDH actually, when packet split descriptor is used, it
> doesn't check whether DH exceeds DLEN?
>

When the packet split feature is used (i.e., count > 1) this patch
basically sets RDH=RDT in case the increment would exceed RDT. The
next iteration should detect that RDH equals RDT in
e1000e_ring_empty(), and exit the loop right before pci_dma_read(). On
the other hand RDH is set to zero if it exceeds DLEN in
e1000e_ring_advance() so we should be fine in either case, unless I'm
missing something?


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

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang]

On 2020/11/27 下午10:49, Mauro Matteo Cascella wrote:
> On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>> value accordingly.
>>>>>> Can this patch solve this issue in another way?
>>>>>>
>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>> So if RDT is odd, it looks to me the following codes in
>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>
>>>>
>>>>            base = e1000e_ring_head_descr(core, rxi);
>>>>
>>>>            pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>
>>>> Otherwise e1000e may try to read out of descriptor ring.
>>> Sorry, I'm not sure I understand what you mean. Isn't the base address
>>> computed from RDH? How can e1000e read out of the descriptor ring if
>>> RDT is odd?
>>>
>>>> Thanks
>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>> value accordingly.
>>>>>> Can this patch solve this issue in another way?
>>>>>>
>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>> So if RDT is odd, it looks to me the following codes in
>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>
>>>>
>>>>            base = e1000e_ring_head_descr(core, rxi);
>>>>
>>>>            pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>
>>>> Otherwise e1000e may try to read out of descriptor ring.
>>>>
>>>> Thanks
>>
>> Sorry, I meant RDH actually, when packet split descriptor is used, it
>> doesn't check whether DH exceeds DLEN?
>>
> When the packet split feature is used (i.e., count > 1) this patch
> basically sets RDH=RDT in case the increment would exceed RDT.


Can software set RDH to an odd value? If not, I think we are probably fine.

Thanks


> The
> next iteration should detect that RDH equals RDT in
> e1000e_ring_empty(), and exit the loop right before pci_dma_read(). On
> the other hand RDH is set to zero if it exceeds DLEN in
> e1000e_ring_advance() so we should be fine in either case, unless I'm
> missing something?
>
>
> Thank you for your time,
> --
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0
>

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Mauro Matteo Cascella]

On Mon, Nov 30, 2020 at 3:58 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2020/11/27 下午10:49, Mauro Matteo Cascella wrote:
> > On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
> >>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> >>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>>>>>> value accordingly.
> >>>>>> Can this patch solve this issue in another way?
> >>>>>>
> >>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>>>>>
> >>>>>> Thanks
> >>>>>>
> >>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
> >>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
> >>>> So if RDT is odd, it looks to me the following codes in
> >>>> e1000e_write_packet_to_guest() needs to be fixed as well.
> >>>>
> >>>>
> >>>>            base = e1000e_ring_head_descr(core, rxi);
> >>>>
> >>>>            pci_dma_read(d, base, &desc, core->rx_desc_len);
> >>>>
> >>>> Otherwise e1000e may try to read out of descriptor ring.
> >>> Sorry, I'm not sure I understand what you mean. Isn't the base address
> >>> computed from RDH? How can e1000e read out of the descriptor ring if
> >>> RDT is odd?
> >>>
> >>>> Thanks
> >>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
> >>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
> >>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
> >>>>>>> value accordingly.
> >>>>>> Can this patch solve this issue in another way?
> >>>>>>
> >>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
> >>>>>>
> >>>>>> Thanks
> >>>>>>
> >>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
> >>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
> >>>> So if RDT is odd, it looks to me the following codes in
> >>>> e1000e_write_packet_to_guest() needs to be fixed as well.
> >>>>
> >>>>
> >>>>            base = e1000e_ring_head_descr(core, rxi);
> >>>>
> >>>>            pci_dma_read(d, base, &desc, core->rx_desc_len);
> >>>>
> >>>> Otherwise e1000e may try to read out of descriptor ring.
> >>>>
> >>>> Thanks
> >>
> >> Sorry, I meant RDH actually, when packet split descriptor is used, it
> >> doesn't check whether DH exceeds DLEN?
> >>
> > When the packet split feature is used (i.e., count > 1) this patch
> > basically sets RDH=RDT in case the increment would exceed RDT.
>
>
> Can software set RDH to an odd value? If not, I think we are probably fine.
>
> Thanks
>

Honestly I don't know the answer to your question, my guess is that it
may be possible for a malicious/bogus guest to set RDH the same way as
RDT.

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

Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance()

[Jason Wang]

On 2020/11/30 下午10:12, Mauro Matteo Cascella wrote:
> On Mon, Nov 30, 2020 at 3:58 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2020/11/27 下午10:49, Mauro Matteo Cascella wrote:
>>> On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote:
>>>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>>>> value accordingly.
>>>>>>>> Can this patch solve this issue in another way?
>>>>>>>>
>>>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>>>> So if RDT is odd, it looks to me the following codes in
>>>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>>>
>>>>>>
>>>>>>             base = e1000e_ring_head_descr(core, rxi);
>>>>>>
>>>>>>             pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>>>
>>>>>> Otherwise e1000e may try to read out of descriptor ring.
>>>>> Sorry, I'm not sure I understand what you mean. Isn't the base address
>>>>> computed from RDH? How can e1000e read out of the descriptor ring if
>>>>> RDT is odd?
>>>>>
>>>>>> Thanks
>>>>> On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>> On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote:
>>>>>>> On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasowang@redhat.com> wrote:
>>>>>>>> On 2020/11/13 下午6:31, 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 not exceed RDT in a single incremental step, adjusting the count
>>>>>>>>> value accordingly.
>>>>>>>> Can this patch solve this issue in another way?
>>>>>>>>
>>>>>>>> https://patchew.org/QEMU/20201111130636.2208620-1-ppandit@redhat.com/
>>>>>>>>
>>>>>>>> Thanks
>>>>>>>>
>>>>>>> Yes, it does work nicely. Still, I think this patch is useful to avoid
>>>>>>> possible inconsistent state in e1000e_ring_advance() when count > 1.
>>>>>> So if RDT is odd, it looks to me the following codes in
>>>>>> e1000e_write_packet_to_guest() needs to be fixed as well.
>>>>>>
>>>>>>
>>>>>>             base = e1000e_ring_head_descr(core, rxi);
>>>>>>
>>>>>>             pci_dma_read(d, base, &desc, core->rx_desc_len);
>>>>>>
>>>>>> Otherwise e1000e may try to read out of descriptor ring.
>>>>>>
>>>>>> Thanks
>>>> Sorry, I meant RDH actually, when packet split descriptor is used, it
>>>> doesn't check whether DH exceeds DLEN?
>>>>
>>> When the packet split feature is used (i.e., count > 1) this patch
>>> basically sets RDH=RDT in case the increment would exceed RDT.
>>
>> Can software set RDH to an odd value? If not, I think we are probably fine.
>>
>> Thanks
>>
> Honestly I don't know the answer to your question, my guess is that it
> may be possible for a malicious/bogus guest to set RDH the same way as
> RDT.
>
> Thank you,
> --
> Mauro Matteo Cascella
> Red Hat Product Security
> PGP-Key ID: BB3410B0
>
>

Then I think we should fix that.

Thanks