[PATCH] e1000: fix tx re-entrancy problem

[PATCH] e1000: fix tx re-entrancy problem

[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>
---
 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);
 }
 
-- 
2.31.1

Re: [PATCH] e1000: fix tx re-entrancy problem

[Jason Wang]

在 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);
>   }
>   

Re: [PATCH] e1000: fix tx re-entrancy problem

[Philippe Mathieu-Daudé]

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?

Re: [PATCH] e1000: fix tx re-entrancy problem

[Jon Maloy]

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

Re: [PATCH] e1000: fix tx re-entrancy problem

[Philippe Mathieu-Daudé]

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.

Re: [PATCH] e1000: fix tx re-entrancy problem

[Alexander Bulekov]

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

Re: [PATCH] e1000: fix tx re-entrancy problem

[Jon Maloy]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

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
>

[-- Attachment #2: e1000-loop.sh.txt --]
[-- Type: text/plain, Size: 1538 bytes --]

#!/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