[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000
From: |
Mauro Matteo Cascella |
Subject: |
Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() |
Date: |
Mon, 23 Nov 2020 22:30:12 +0100 |
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?
> >>
> >> 20201111130636.2208620-1-ppandit@redhat.com/">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?
> >>
> >> 20201111130636.2208620-1-ppandit@redhat.com/">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
- [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Mauro Matteo Cascella, 2020/11/13
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Jason Wang, 2020/11/17
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Mauro Matteo Cascella, 2020/11/18
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Jason Wang, 2020/11/19
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(),
Mauro Matteo Cascella <=
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Jason Wang, 2020/11/27
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Mauro Matteo Cascella, 2020/11/27
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Jason Wang, 2020/11/29
- Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance(), Mauro Matteo Cascella, 2020/11/30