On Mon, Nov 30, 2020 at 3:58 AM Jason Wang <jasow...@redhat.com> wrote: > > > On 2020/11/27 下午10:49, Mauro Matteo Cascella wrote: > > On Fri, Nov 27, 2020 at 6:21 AM Jason Wang <jasow...@redhat.com> wrote: > >> > >> On 2020/11/24 上午5:30, Mauro Matteo Cascella wrote: > >>> 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 > >> > >> 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
