* [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() @ 2020-11-13 10:31 Mauro Matteo Cascella 2020-11-18 3:56 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Mauro Matteo Cascella @ 2020-11-13 10:31 UTC (permalink / raw) To: qemu-devel Cc: mcascell, dmitry.fleytman, gaoning.pgn, jasowang, lersek, 330cjfdn 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-13 10:31 [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() Mauro Matteo Cascella @ 2020-11-18 3:56 ` Jason Wang 2020-11-18 8:53 ` Mauro Matteo Cascella 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2020-11-18 3:56 UTC (permalink / raw) To: Mauro Matteo Cascella, qemu-devel Cc: gaoning.pgn, 330cjfdn, dmitry.fleytman, lersek 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-18 3:56 ` Jason Wang @ 2020-11-18 8:53 ` Mauro Matteo Cascella 2020-11-19 5:57 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Mauro Matteo Cascella @ 2020-11-18 8:53 UTC (permalink / raw) To: Jason Wang Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-18 8:53 ` Mauro Matteo Cascella @ 2020-11-19 5:57 ` Jason Wang 2020-11-23 21:30 ` Mauro Matteo Cascella 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2020-11-19 5:57 UTC (permalink / raw) To: Mauro Matteo Cascella Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-19 5:57 ` Jason Wang @ 2020-11-23 21:30 ` Mauro Matteo Cascella 2020-11-27 5:21 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Mauro Matteo Cascella @ 2020-11-23 21:30 UTC (permalink / raw) To: Jason Wang Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-23 21:30 ` Mauro Matteo Cascella @ 2020-11-27 5:21 ` Jason Wang 2020-11-27 14:49 ` Mauro Matteo Cascella 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2020-11-27 5:21 UTC (permalink / raw) To: Mauro Matteo Cascella Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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, > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-27 5:21 ` Jason Wang @ 2020-11-27 14:49 ` Mauro Matteo Cascella 2020-11-30 2:58 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Mauro Matteo Cascella @ 2020-11-27 14:49 UTC (permalink / raw) To: Jason Wang Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-27 14:49 ` Mauro Matteo Cascella @ 2020-11-30 2:58 ` Jason Wang 2020-11-30 14:12 ` Mauro Matteo Cascella 0 siblings, 1 reply; 10+ messages in thread From: Jason Wang @ 2020-11-30 2:58 UTC (permalink / raw) To: Mauro Matteo Cascella Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-30 2:58 ` Jason Wang @ 2020-11-30 14:12 ` Mauro Matteo Cascella 2020-12-01 5:42 ` Jason Wang 0 siblings, 1 reply; 10+ messages in thread From: Mauro Matteo Cascella @ 2020-11-30 14:12 UTC (permalink / raw) To: Jason Wang Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() 2020-11-30 14:12 ` Mauro Matteo Cascella @ 2020-12-01 5:42 ` Jason Wang 0 siblings, 0 replies; 10+ messages in thread From: Jason Wang @ 2020-12-01 5:42 UTC (permalink / raw) To: Mauro Matteo Cascella Cc: gaoning.pgn, 330cjfdn, Dmitry Fleytman, Laszlo Ersek, QEMU Developers 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-01 5:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-13 10:31 [PATCH v2] net/e1000e_core: adjust count if RDH exceeds RDT in e1000e_ring_advance() Mauro Matteo Cascella 2020-11-18 3:56 ` Jason Wang 2020-11-18 8:53 ` Mauro Matteo Cascella 2020-11-19 5:57 ` Jason Wang 2020-11-23 21:30 ` Mauro Matteo Cascella 2020-11-27 5:21 ` Jason Wang 2020-11-27 14:49 ` Mauro Matteo Cascella 2020-11-30 2:58 ` Jason Wang 2020-11-30 14:12 ` Mauro Matteo Cascella 2020-12-01 5:42 ` Jason Wang
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.