On Thu, Nov 19, 2020 at 6:57 AM Jason Wang <jasow...@redhat.com> wrote: > > > On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote: > > On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasow...@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-ppan...@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 <jasow...@redhat.com> wrote: > > > On 2020/11/18 下午4:53, Mauro Matteo Cascella wrote: > > On Wed, Nov 18, 2020 at 4:56 AM Jason Wang <jasow...@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-ppan...@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
