qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] e1000: fix tx re-entrancy problem
@ 2021-10-21 16:10 Jon Maloy
  2021-10-27  4:40 ` Jason Wang
  2021-12-16  9:36 ` Philippe Mathieu-Daudé
  0 siblings, 2 replies; 7+ messages in thread
From: Jon Maloy @ 2021-10-21 16:10 UTC (permalink / raw)
  To: qemu-devel; +Cc: jmaloy

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



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-10-21 16:10 [PATCH] e1000: fix tx re-entrancy problem Jon Maloy
@ 2021-10-27  4:40 ` Jason Wang
  2021-12-16  9:36 ` Philippe Mathieu-Daudé
  1 sibling, 0 replies; 7+ messages in thread
From: Jason Wang @ 2021-10-27  4:40 UTC (permalink / raw)
  To: Jon Maloy, qemu-devel


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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-10-21 16:10 [PATCH] e1000: fix tx re-entrancy problem Jon Maloy
  2021-10-27  4:40 ` Jason Wang
@ 2021-12-16  9:36 ` Philippe Mathieu-Daudé
  2021-12-16 15:51   ` Jon Maloy
  1 sibling, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-16  9:36 UTC (permalink / raw)
  To: Jon Maloy, qemu-devel, Jason Wang, Alexander Bulekov

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?



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-12-16  9:36 ` Philippe Mathieu-Daudé
@ 2021-12-16 15:51   ` Jon Maloy
  2021-12-16 18:35     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 7+ messages in thread
From: Jon Maloy @ 2021-12-16 15:51 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel, Jason Wang, Alexander Bulekov



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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-12-16 15:51   ` Jon Maloy
@ 2021-12-16 18:35     ` Philippe Mathieu-Daudé
  2021-12-16 19:01       ` Alexander Bulekov
  0 siblings, 1 reply; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-12-16 18:35 UTC (permalink / raw)
  To: Jon Maloy, qemu-devel, Jason Wang, Alexander Bulekov

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.



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-12-16 18:35     ` Philippe Mathieu-Daudé
@ 2021-12-16 19:01       ` Alexander Bulekov
  2021-12-16 20:22         ` Jon Maloy
  0 siblings, 1 reply; 7+ messages in thread
From: Alexander Bulekov @ 2021-12-16 19:01 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Jon Maloy, Jason Wang, qemu-devel

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] e1000: fix tx re-entrancy problem
  2021-12-16 19:01       ` Alexander Bulekov
@ 2021-12-16 20:22         ` Jon Maloy
  0 siblings, 0 replies; 7+ messages in thread
From: Jon Maloy @ 2021-12-16 20:22 UTC (permalink / raw)
  To: Alexander Bulekov, Philippe Mathieu-Daudé; +Cc: Jason Wang, qemu-devel

[-- 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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-12-16 20:19 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-21 16:10 [PATCH] e1000: fix tx re-entrancy problem Jon Maloy
2021-10-27  4:40 ` Jason Wang
2021-12-16  9:36 ` Philippe Mathieu-Daudé
2021-12-16 15:51   ` Jon Maloy
2021-12-16 18:35     ` Philippe Mathieu-Daudé
2021-12-16 19:01       ` Alexander Bulekov
2021-12-16 20:22         ` Jon Maloy

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).