| Message ID | 20211021161047.578751-1-jmaloy@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | e1000: fix tx re-entrancy problem | expand |
在 2021/10/22 上午12:10, Jon Maloy 写道: > The fact that the MMIO handler is not re-entrant causes an infinite > loop under certain conditions: > > Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX > > We now eliminate the effect of this problem locally in e1000, by adding > a boolean in struct E1000State indicating when the TX side is busy. This > will cause any entering new call to return early instead of interfering > with the ongoing work, and eliminates any risk of looping. > > This is intended to address CVE-2021-20257. > > Signed-off-by: Jon Maloy <jmaloy@redhat.com> Applied. Thanks > --- > hw/net/e1000.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index a30546c5d5..f5bc81296d 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -107,6 +107,7 @@ struct E1000State_st { > e1000x_txd_props props; > e1000x_txd_props tso_props; > uint16_t tso_frames; > + bool busy; > } tx; > > struct { > @@ -763,6 +764,11 @@ start_xmit(E1000State *s) > return; > } > > + if (s->tx.busy) { > + return; > + } > + s->tx.busy = true; > + > while (s->mac_reg[TDH] != s->mac_reg[TDT]) { > base = tx_desc_base(s) + > sizeof(struct e1000_tx_desc) * s->mac_reg[TDH]; > @@ -789,6 +795,7 @@ start_xmit(E1000State *s) > break; > } > } > + s->tx.busy = false; > set_ics(s, 0, cause); > } >
Hi Jon, On 10/21/21 18:10, Jon Maloy wrote: > The fact that the MMIO handler is not re-entrant causes an infinite > loop under certain conditions: > > Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX > > We now eliminate the effect of this problem locally in e1000, by adding > a boolean in struct E1000State indicating when the TX side is busy. This > will cause any entering new call to return early instead of interfering > with the ongoing work, and eliminates any risk of looping. > > This is intended to address CVE-2021-20257. > > Signed-off-by: Jon Maloy <jmaloy@redhat.com> > --- > hw/net/e1000.c | 7 +++++++ > 1 file changed, 7 insertions(+) I can not find the reproducer in the repository, have you sent one?
On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: > Hi Jon, > > On 10/21/21 18:10, Jon Maloy wrote: >> The fact that the MMIO handler is not re-entrant causes an infinite >> loop under certain conditions: >> >> Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX >> >> We now eliminate the effect of this problem locally in e1000, by adding >> a boolean in struct E1000State indicating when the TX side is busy. This >> will cause any entering new call to return early instead of interfering >> with the ongoing work, and eliminates any risk of looping. >> >> This is intended to address CVE-2021-20257. >> >> Signed-off-by: Jon Maloy <jmaloy@redhat.com> >> --- >> hw/net/e1000.c | 7 +++++++ >> 1 file changed, 7 insertions(+) > I can not find the reproducer in the repository, have you sent one? > No, I did not add it to the repo. It was referenced from the tracker BZ, but I was unable to get access back then. It ended up with that I had it sent by mail to me directly. What is your question? Is it that it should be in the repo, or that you cannot find it? ///jon
On 12/16/21 16:51, Jon Maloy wrote: > On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: >> Hi Jon, >> >> On 10/21/21 18:10, Jon Maloy wrote: >>> The fact that the MMIO handler is not re-entrant causes an infinite >>> loop under certain conditions: >>> >>> Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX >>> >>> We now eliminate the effect of this problem locally in e1000, by adding >>> a boolean in struct E1000State indicating when the TX side is busy. This >>> will cause any entering new call to return early instead of interfering >>> with the ongoing work, and eliminates any risk of looping. >>> >>> This is intended to address CVE-2021-20257. >>> >>> Signed-off-by: Jon Maloy <jmaloy@redhat.com> >>> --- >>> hw/net/e1000.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >> I can not find the reproducer in the repository, have you sent one? >> > No, I did not add it to the repo. > It was referenced from the tracker BZ, but I was unable to get access > back then. > It ended up with that I had it sent by mail to me directly. > > What is your question? Is it that it should be in the repo, or that you > cannot find it? Well I'd like to reproduce the bug, but first I can not find it ;) Having such reproducer committed along with the fix help catching future regressions if we refactor code elsewhere.
On 211216 1935, Philippe Mathieu-Daudé wrote: > On 12/16/21 16:51, Jon Maloy wrote: > > On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: > >> Hi Jon, > >> > >> On 10/21/21 18:10, Jon Maloy wrote: > >>> The fact that the MMIO handler is not re-entrant causes an infinite > >>> loop under certain conditions: > >>> > >>> Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX > >>> > >>> We now eliminate the effect of this problem locally in e1000, by adding > >>> a boolean in struct E1000State indicating when the TX side is busy. This > >>> will cause any entering new call to return early instead of interfering > >>> with the ongoing work, and eliminates any risk of looping. > >>> > >>> This is intended to address CVE-2021-20257. > >>> > >>> Signed-off-by: Jon Maloy <jmaloy@redhat.com> > >>> --- > >>> hw/net/e1000.c | 7 +++++++ > >>> 1 file changed, 7 insertions(+) > >> I can not find the reproducer in the repository, have you sent one? > >> > > No, I did not add it to the repo. > > It was referenced from the tracker BZ, but I was unable to get access > > back then. > > It ended up with that I had it sent by mail to me directly. > > > > What is your question? Is it that it should be in the repo, or that you > > cannot find it? > > Well I'd like to reproduce the bug, but first I can not find it ;) > Having such reproducer committed along with the fix help catching > future regressions if we refactor code elsewhere. > Blind guess, but assuming write to TDT == set_tctl, maybe this one? https://bugs.launchpad.net/qemu/+bug/1917082
This was the one I received. ///jon On 12/16/21 14:01, Alexander Bulekov wrote: > On 211216 1935, Philippe Mathieu-Daudé wrote: >> On 12/16/21 16:51, Jon Maloy wrote: >>> On 12/16/21 04:36, Philippe Mathieu-Daudé wrote: >>>> Hi Jon, >>>> >>>> On 10/21/21 18:10, Jon Maloy wrote: >>>>> The fact that the MMIO handler is not re-entrant causes an infinite >>>>> loop under certain conditions: >>>>> >>>>> Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX >>>>> >>>>> We now eliminate the effect of this problem locally in e1000, by adding >>>>> a boolean in struct E1000State indicating when the TX side is busy. This >>>>> will cause any entering new call to return early instead of interfering >>>>> with the ongoing work, and eliminates any risk of looping. >>>>> >>>>> This is intended to address CVE-2021-20257. >>>>> >>>>> Signed-off-by: Jon Maloy <jmaloy@redhat.com> >>>>> --- >>>>> hw/net/e1000.c | 7 +++++++ >>>>> 1 file changed, 7 insertions(+) >>>> I can not find the reproducer in the repository, have you sent one? >>>> >>> No, I did not add it to the repo. >>> It was referenced from the tracker BZ, but I was unable to get access >>> back then. >>> It ended up with that I had it sent by mail to me directly. >>> >>> What is your question? Is it that it should be in the repo, or that you >>> cannot find it? >> Well I'd like to reproduce the bug, but first I can not find it ;) >> Having such reproducer committed along with the fix help catching >> future regressions if we refactor code elsewhere. >> > Blind guess, but assuming write to TDT == set_tctl, maybe this one? > https://bugs.launchpad.net/qemu/+bug/1917082 > #!/bin/sh cat << EOF > inp clock_step clock_step clock_step clock_step clock_step clock_step outl 0xcf8 0x80000813 outl 0xcfc 0xfffffffe outl 0xcf8 0x80000803 outw 0xcfc 0x66e2 write 0xfe000102 0x1 0xff clock_step writel 0xfe000020 0x420ff00 write 0xfe00280a 0x3 0x2828ff clock_step clock_step clock_step write 0xfe002815 0x4 0x0300ff46 clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step clock_step write 0xf27 0x1 0xff write 0xf98 0x1 0xd5 write 0xf99 0x1 0xd5 write 0xf9b 0x1 0xd5 write 0x1060 0x1 0x17 write 0x1061 0x1 0x38 write 0x1062 0x3 0x00fe00 writel 0xfe0003ff 0x8e8e8e8e write 0xfe00380a 0x3 0x525e03 write 0xfe003818 0x1 0xff EOF ./x86_64-softmmu/qemu-system-x86_64 -display none -machine accel=qtest \ -m 512M -M q35 -nodefaults -device e1000,netdev=net0 \ -netdev user,id=net0 -qtest-log none -qtest stdio < inp
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index a30546c5d5..f5bc81296d 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -107,6 +107,7 @@ struct E1000State_st { e1000x_txd_props props; e1000x_txd_props tso_props; uint16_t tso_frames; + bool busy; } tx; struct { @@ -763,6 +764,11 @@ start_xmit(E1000State *s) return; } + if (s->tx.busy) { + return; + } + s->tx.busy = true; + while (s->mac_reg[TDH] != s->mac_reg[TDT]) { base = tx_desc_base(s) + sizeof(struct e1000_tx_desc) * s->mac_reg[TDH]; @@ -789,6 +795,7 @@ start_xmit(E1000State *s) break; } } + s->tx.busy = false; set_ics(s, 0, cause); }
The fact that the MMIO handler is not re-entrant causes an infinite loop under certain conditions: Guest write to TDT -> Loopback -> RX (DMA to TDT) -> TX We now eliminate the effect of this problem locally in e1000, by adding a boolean in struct E1000State indicating when the TX side is busy. This will cause any entering new call to return early instead of interfering with the ongoing work, and eliminates any risk of looping. This is intended to address CVE-2021-20257. Signed-off-by: Jon Maloy <jmaloy@redhat.com> --- hw/net/e1000.c | 7 +++++++ 1 file changed, 7 insertions(+)