| Message ID | 20201113103113.223239-1-mcascell@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() | expand |
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
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,
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,
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, >
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, >
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
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 >
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
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
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]) {