[PATCH 0/6] Detect reentrant RX casue by loopback

[PATCH 0/6] Detect reentrant RX casue by loopback

[Jason Wang]

Hi All:

Followed by commit 22dc8663d9 ("net: forbid the reentrant RX"), we
still need to fix the issues casued by loopback mode where the NIC
usually it via calling nc->info->receive() directly.

The fix is to introduce new network helper and check the
queue->delivering.

Thanks

Jason Wang (6):
  net: introduce qemu_receive_packet()
  e1000: switch to use qemu_receive_packet() for loopback
  dp8393x: switch to use qemu_receive_packet() for loopback packet
  msf2-mac: switch to use qemu_receive_packet() for loopback
  sungem: switch to use qemu_receive_packet() for loopback
  tx_pkt: switch to use qemu_receive_packet_iov() for loopback

 hw/net/dp8393x.c    |  2 +-
 hw/net/e1000.c      |  2 +-
 hw/net/msf2-emac.c  |  2 +-
 hw/net/net_tx_pkt.c |  2 +-
 hw/net/sungem.c     |  2 +-
 include/net/net.h   |  5 +++++
 include/net/queue.h |  8 ++++++++
 net/net.c           | 38 +++++++++++++++++++++++++++++++-------
 net/queue.c         | 22 ++++++++++++++++++++++
 9 files changed, 71 insertions(+), 12 deletions(-)

-- 
2.25.1

[PATCH 1/6] net: introduce qemu_receive_packet()

[Jason Wang]

Some NIC supports loopback mode and this is done by calling
nc->info->receive() directly which in fact suppresses the effort of
reentrancy check that is done in qemu_net_queue_send().

Unfortunately we can use qemu_net_queue_send() here since for loop
back there's no sender as peer, so this patch introduce a
qemu_receive_packet() which is used for implementing loopback mode
for a NIC with this check.

NIC that supports loopback mode will be converted to this helper.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 include/net/net.h   |  5 +++++
 include/net/queue.h |  8 ++++++++
 net/net.c           | 38 +++++++++++++++++++++++++++++++-------
 net/queue.c         | 22 ++++++++++++++++++++++
 4 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/include/net/net.h b/include/net/net.h
index 919facaad2..65eb8a58c5 100644
--- a/include/net/net.h
+++ b/include/net/net.h
@@ -144,12 +144,17 @@ void *qemu_get_nic_opaque(NetClientState *nc);
 void qemu_del_net_client(NetClientState *nc);
 typedef void (*qemu_nic_foreach)(NICState *nic, void *opaque);
 void qemu_foreach_nic(qemu_nic_foreach func, void *opaque);
+int qemu_can_receive_packet(NetClientState *nc);
 int qemu_can_send_packet(NetClientState *nc);
 ssize_t qemu_sendv_packet(NetClientState *nc, const struct iovec *iov,
                           int iovcnt);
 ssize_t qemu_sendv_packet_async(NetClientState *nc, const struct iovec *iov,
                                 int iovcnt, NetPacketSent *sent_cb);
 ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size);
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf,int size);
+ssize_t qemu_receive_packet_iov(NetClientState *nc,
+                                const struct iovec *iov,
+                                int iovcnt);
 ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size);
 ssize_t qemu_send_packet_async(NetClientState *nc, const uint8_t *buf,
                                int size, NetPacketSent *sent_cb);
diff --git a/include/net/queue.h b/include/net/queue.h
index c0269bb1dc..9f2f289d77 100644
--- a/include/net/queue.h
+++ b/include/net/queue.h
@@ -55,6 +55,14 @@ void qemu_net_queue_append_iov(NetQueue *queue,
 
 void qemu_del_net_queue(NetQueue *queue);
 
+ssize_t qemu_net_queue_receive(NetQueue *queue,
+                               const uint8_t *data,
+                               size_t size);
+
+ssize_t qemu_net_queue_receive_iov(NetQueue *queue,
+                                   const struct iovec *iov,
+                                   int iovcnt);
+
 ssize_t qemu_net_queue_send(NetQueue *queue,
                             NetClientState *sender,
                             unsigned flags,
diff --git a/net/net.c b/net/net.c
index e1035f21d1..6e470133ad 100644
--- a/net/net.c
+++ b/net/net.c
@@ -528,6 +528,17 @@ int qemu_set_vnet_be(NetClientState *nc, bool is_be)
 #endif
 }
 
+int qemu_can_receive_packet(NetClientState *nc)
+{
+    if (nc->receive_disabled) {
+        return 0;
+    } else if (nc->info->can_receive &&
+               !nc->info->can_receive(nc)) {
+        return 0;
+    }
+    return 1;
+}
+
 int qemu_can_send_packet(NetClientState *sender)
 {
     int vm_running = runstate_is_running();
@@ -540,13 +551,7 @@ int qemu_can_send_packet(NetClientState *sender)
         return 1;
     }
 
-    if (sender->peer->receive_disabled) {
-        return 0;
-    } else if (sender->peer->info->can_receive &&
-               !sender->peer->info->can_receive(sender->peer)) {
-        return 0;
-    }
-    return 1;
+    return qemu_can_receive_packet(sender->peer);
 }
 
 static ssize_t filter_receive_iov(NetClientState *nc,
@@ -679,6 +684,25 @@ ssize_t qemu_send_packet(NetClientState *nc, const uint8_t *buf, int size)
     return qemu_send_packet_async(nc, buf, size, NULL);
 }
 
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf, int size)
+{
+    if (!qemu_can_receive_packet(nc)) {
+        return 0;
+    }
+
+    return qemu_net_queue_receive(nc->incoming_queue, buf, size);
+}
+
+ssize_t qemu_receive_packet_iov(NetClientState *nc, const struct iovec *iov,
+                                int iovcnt)
+{
+    if (!qemu_can_receive_packet(nc)) {
+        return 0;
+    }
+
+    return qemu_net_queue_receive_iov(nc->incoming_queue, iov, iovcnt);
+}
+
 ssize_t qemu_send_packet_raw(NetClientState *nc, const uint8_t *buf, int size)
 {
     return qemu_send_packet_async_with_flags(nc, QEMU_NET_PACKET_FLAG_RAW,
diff --git a/net/queue.c b/net/queue.c
index 19e32c80fd..c872d51df8 100644
--- a/net/queue.c
+++ b/net/queue.c
@@ -182,6 +182,28 @@ static ssize_t qemu_net_queue_deliver_iov(NetQueue *queue,
     return ret;
 }
 
+ssize_t qemu_net_queue_receive(NetQueue *queue,
+                               const uint8_t *data,
+                               size_t size)
+{
+    if (queue->delivering) {
+        return 0;
+    }
+
+    return qemu_net_queue_deliver(queue, NULL, 0, data, size);
+}
+
+ssize_t qemu_net_queue_receive_iov(NetQueue *queue,
+                                   const struct iovec *iov,
+                                   int iovcnt)
+{
+    if (queue->delivering) {
+        return 0;
+    }
+
+    return qemu_net_queue_deliver_iov(queue, NULL, 0, iov, iovcnt);
+}
+
 ssize_t qemu_net_queue_send(NetQueue *queue,
                             NetClientState *sender,
                             unsigned flags,
-- 
2.25.1

[PATCH 2/6] e1000: switch to use qemu_receive_packet() for loopback

[Jason Wang]

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/e1000.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/e1000.c b/hw/net/e1000.c
index 4345d863e6..4f75b44cfc 100644
--- a/hw/net/e1000.c
+++ b/hw/net/e1000.c
@@ -546,7 +546,7 @@ e1000_send_packet(E1000State *s, const uint8_t *buf, int size)
 
     NetClientState *nc = qemu_get_queue(s->nic);
     if (s->phy_reg[PHY_CTRL] & MII_CR_LOOPBACK) {
-        nc->info->receive(nc, buf, size);
+        qemu_receive_packet(nc, buf, size);
     } else {
         qemu_send_packet(nc, buf, size);
     }
-- 
2.25.1

[PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

[Jason Wang]

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/dp8393x.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
index 205c0decc5..019d4fe435 100644
--- a/hw/net/dp8393x.c
+++ b/hw/net/dp8393x.c
@@ -506,7 +506,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
             s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
             if (nc->info->can_receive(nc)) {
                 s->loopback_packet = 1;
-                nc->info->receive(nc, s->tx_buffer, tx_len);
+                qemu_receice_packet(nc, s->tx_buffer, tx_Len);
             }
         } else {
             /* Transmit packet */
-- 
2.25.1

[PATCH 4/6] msf2-mac: switch to use qemu_receive_packet() for loopback

[Jason Wang]

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/msf2-emac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/msf2-emac.c b/hw/net/msf2-emac.c
index 32ba9e8412..3e6206044f 100644
--- a/hw/net/msf2-emac.c
+++ b/hw/net/msf2-emac.c
@@ -158,7 +158,7 @@ static void msf2_dma_tx(MSF2EmacState *s)
          * R_CFG1 bit 0 is set.
          */
         if (s->regs[R_CFG1] & R_CFG1_LB_EN_MASK) {
-            nc->info->receive(nc, buf, size);
+            qemu_receive_packet(nc, buf, size);
         } else {
             qemu_send_packet(nc, buf, size);
         }
-- 
2.25.1

[PATCH 5/6] sungem: switch to use qemu_receive_packet() for loopback

[Jason Wang]

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/sungem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/sungem.c b/hw/net/sungem.c
index 33c3722df6..3684a4d733 100644
--- a/hw/net/sungem.c
+++ b/hw/net/sungem.c
@@ -306,7 +306,7 @@ static void sungem_send_packet(SunGEMState *s, const uint8_t *buf,
     NetClientState *nc = qemu_get_queue(s->nic);
 
     if (s->macregs[MAC_XIFCFG >> 2] & MAC_XIFCFG_LBCK) {
-        nc->info->receive(nc, buf, size);
+        qemu_receive_packet(nc, buf, size);
     } else {
         qemu_send_packet(nc, buf, size);
     }
-- 
2.25.1

[PATCH 6/6] tx_pkt: switch to use qemu_receive_packet_iov() for loopback

[Jason Wang]

This patch switches to use qemu_receive_receive_iov() which can detect
reentrancy and return early.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 hw/net/net_tx_pkt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/net_tx_pkt.c b/hw/net/net_tx_pkt.c
index da262edc3e..1f9aa59eca 100644
--- a/hw/net/net_tx_pkt.c
+++ b/hw/net/net_tx_pkt.c
@@ -553,7 +553,7 @@ static inline void net_tx_pkt_sendv(struct NetTxPkt *pkt,
     NetClientState *nc, const struct iovec *iov, int iov_cnt)
 {
     if (pkt->is_loopback) {
-        nc->info->receive_iov(nc, iov, iov_cnt);
+        qemu_receive_packet_iov(nc, iov, iov_cnt);
     } else {
         qemu_sendv_packet(nc, iov, iov_cnt);
     }
-- 
2.25.1

Re: [PATCH 0/6] Detect reentrant RX casue by loopback

[no-reply]

Patchew URL: https://patchew.org/QEMU/20210224055401.492407-1-jasowang@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20210224055401.492407-1-jasowang@redhat.com
Subject: [PATCH 0/6] Detect reentrant RX casue by loopback

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20210224055401.492407-1-jasowang@redhat.com -> patchew/20210224055401.492407-1-jasowang@redhat.com
Switched to a new branch 'test'
6a41d09 tx_pkt: switch to use qemu_receive_packet_iov() for loopback
5480e55 sungem: switch to use qemu_receive_packet() for loopback
f35fab6 msf2-mac: switch to use qemu_receive_packet() for loopback
1dc29cf dp8393x: switch to use qemu_receive_packet() for loopback packet
6c79834 e1000: switch to use qemu_receive_packet() for loopback
04660c0 net: introduce qemu_receive_packet()

=== OUTPUT BEGIN ===
1/6 Checking commit 04660c06d88e (net: introduce qemu_receive_packet())
ERROR: space required after that ',' (ctx:VxV)
#42: FILE: include/net/net.h:154:
+ssize_t qemu_receive_packet(NetClientState *nc, const uint8_t *buf,int size);
                                                                   ^

total: 1 errors, 0 warnings, 115 lines checked

Patch 1/6 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

2/6 Checking commit 6c7983468e94 (e1000: switch to use qemu_receive_packet() for loopback)
3/6 Checking commit 1dc29cfa3117 (dp8393x: switch to use qemu_receive_packet() for loopback packet)
4/6 Checking commit f35fab6cb737 (msf2-mac: switch to use qemu_receive_packet() for loopback)
5/6 Checking commit 5480e5557678 (sungem: switch to use qemu_receive_packet() for loopback)
6/6 Checking commit 6a41d093a705 (tx_pkt: switch to use qemu_receive_packet_iov() for loopback)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20210224055401.492407-1-jasowang@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

[Stefan Weil]

Am 24.02.21 um 06:53 schrieb Jason Wang:

> This patch switches to use qemu_receive_packet() which can detect
> reentrancy and return early.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>   hw/net/dp8393x.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
> index 205c0decc5..019d4fe435 100644
> --- a/hw/net/dp8393x.c
> +++ b/hw/net/dp8393x.c
> @@ -506,7 +506,7 @@ static void dp8393x_do_transmit_packets(dp8393xState *s)
>               s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>               if (nc->info->can_receive(nc)) {
>                   s->loopback_packet = 1;
> -                nc->info->receive(nc, s->tx_buffer, tx_len);
> +                qemu_receice_packet(nc, s->tx_buffer, tx_Len);


Did you test compilation of that code? It looks like a typo ...

Regards

Stefan W.

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Philippe Mathieu-Daudé]

On 2/24/21 6:53 AM, Jason Wang wrote:
> Some NIC supports loopback mode and this is done by calling
> nc->info->receive() directly which in fact suppresses the effort of
> reentrancy check that is done in qemu_net_queue_send().
> 
> Unfortunately we can use qemu_net_queue_send() here since for loop
> back there's no sender as peer, so this patch introduce a
> qemu_receive_packet() which is used for implementing loopback mode
> for a NIC with this check.

IIUC the guest could trigger an infinite loop and brick the emulated
device model. Likely exhausting the stack, so either SEGV by
corruption or some ENOMEM?

Since this is guest triggerable, shouldn't we contact qemu-security@
list and ask for a CVE for this issue, so distributions can track
the patches to backport in their stable releases? (it seems to be
within the KVM devices boundary).

> 
> NIC that supports loopback mode will be converted to this helper.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  include/net/net.h   |  5 +++++
>  include/net/queue.h |  8 ++++++++
>  net/net.c           | 38 +++++++++++++++++++++++++++++++-------
>  net/queue.c         | 22 ++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 7 deletions(-)

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Jason Wang]

On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
> On 2/24/21 6:53 AM, Jason Wang wrote:
>> Some NIC supports loopback mode and this is done by calling
>> nc->info->receive() directly which in fact suppresses the effort of
>> reentrancy check that is done in qemu_net_queue_send().
>>
>> Unfortunately we can use qemu_net_queue_send() here since for loop
>> back there's no sender as peer, so this patch introduce a
>> qemu_receive_packet() which is used for implementing loopback mode
>> for a NIC with this check.
> IIUC the guest could trigger an infinite loop and brick the emulated
> device model. Likely exhausting the stack, so either SEGV by
> corruption or some ENOMEM?


Yes.


>
> Since this is guest triggerable, shouldn't we contact qemu-security@
> list and ask for a CVE for this issue, so distributions can track
> the patches to backport in their stable releases? (it seems to be
> within the KVM devices boundary).


That's the plan. I discussed this with Prasad before and he promise to 
ask CVE for this.

But it's a knwon issue, the reentrant DMA which has been discussed 
before[1], unfortuantely we don't make any progress. This patch can only 
fix the NIC RX issue.

Thanks

[1] https://mail.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html


>
>> NIC that supports loopback mode will be converted to this helper.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   include/net/net.h   |  5 +++++
>>   include/net/queue.h |  8 ++++++++
>>   net/net.c           | 38 +++++++++++++++++++++++++++++++-------
>>   net/queue.c         | 22 ++++++++++++++++++++++
>>   4 files changed, 66 insertions(+), 7 deletions(-)

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Philippe Mathieu-Daudé]

On 2/24/21 2:17 PM, Jason Wang wrote:
> 
> On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
>> On 2/24/21 6:53 AM, Jason Wang wrote:
>>> Some NIC supports loopback mode and this is done by calling
>>> nc->info->receive() directly which in fact suppresses the effort of
>>> reentrancy check that is done in qemu_net_queue_send().
>>>
>>> Unfortunately we can use qemu_net_queue_send() here since for loop
>>> back there's no sender as peer, so this patch introduce a
>>> qemu_receive_packet() which is used for implementing loopback mode
>>> for a NIC with this check.
>> IIUC the guest could trigger an infinite loop and brick the emulated
>> device model. Likely exhausting the stack, so either SEGV by
>> corruption or some ENOMEM?
> 
> 
> Yes.
> 
> 
>>
>> Since this is guest triggerable, shouldn't we contact qemu-security@
>> list and ask for a CVE for this issue, so distributions can track
>> the patches to backport in their stable releases? (it seems to be
>> within the KVM devices boundary).
> 
> 
> That's the plan. I discussed this with Prasad before and he promise to
> ask CVE for this.

Good! We just need to be sure to amend the CVE number to the patches
before committing them.

> 
> But it's a knwon issue, the reentrant DMA which has been discussed
> before[1], unfortuantely we don't make any progress. This patch can only
> fix the NIC RX issue.
> 
> Thanks
> 
> [1] https://mail.gnu.org/archive/html/qemu-devel/2020-09/msg00906.html

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[P J P]

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

+-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+
| On 2/24/21 2:17 PM, Jason Wang wrote:
| > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
| >> IIUC the guest could trigger an infinite loop and brick the emulated 
| >> device model. Likely exhausting the stack, so either SEGV by corruption 
| >> or some ENOMEM?
| > 
| > Yes.
| >>
| >> Since this is guest triggerable, shouldn't we contact qemu-security@ list 
| >> and ask for a CVE for this issue, so distributions can track the patches 
| >> to backport in their stable releases? (it seems to be within the KVM 
| >> devices boundary).
| > 
| > 
| > That's the plan. I discussed this with Prasad before and he promise to
| > ask CVE for this.

'CVE-2021-3416' is assigned to this issue by Red Hat Inc.

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Philippe Mathieu-Daudé]

On 2/24/21 6:53 AM, Jason Wang wrote:
> Some NIC supports loopback mode and this is done by calling
> nc->info->receive() directly which in fact suppresses the effort of
> reentrancy check that is done in qemu_net_queue_send().
> 
> Unfortunately we can use qemu_net_queue_send() here since for loop
> back there's no sender as peer, so this patch introduce a
> qemu_receive_packet() which is used for implementing loopback mode
> for a NIC with this check.
> 
> NIC that supports loopback mode will be converted to this helper.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  include/net/net.h   |  5 +++++
>  include/net/queue.h |  8 ++++++++
>  net/net.c           | 38 +++++++++++++++++++++++++++++++-------
>  net/queue.c         | 22 ++++++++++++++++++++++
>  4 files changed, 66 insertions(+), 7 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 2/6] e1000: switch to use qemu_receive_packet() for loopback

[Philippe Mathieu-Daudé]

On 2/24/21 6:53 AM, Jason Wang wrote:
> This patch switches to use qemu_receive_packet() which can detect
> reentrancy and return early.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/e1000.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 4/6] msf2-mac: switch to use qemu_receive_packet() for loopback

[Philippe Mathieu-Daudé]

On 2/24/21 6:53 AM, Jason Wang wrote:
> This patch switches to use qemu_receive_packet() which can detect
> reentrancy and return early.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/msf2-emac.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 5/6] sungem: switch to use qemu_receive_packet() for loopback

[Philippe Mathieu-Daudé]

On 2/24/21 6:54 AM, Jason Wang wrote:
> This patch switches to use qemu_receive_packet() which can detect
> reentrancy and return early.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/sungem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 6/6] tx_pkt: switch to use qemu_receive_packet_iov() for loopback

[Philippe Mathieu-Daudé]

On 2/24/21 6:54 AM, Jason Wang wrote:
> This patch switches to use qemu_receive_receive_iov() which can detect
> reentrancy and return early.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  hw/net/net_tx_pkt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

[Philippe Mathieu-Daudé]

On 2/24/21 7:13 AM, Stefan Weil wrote:
> Am 24.02.21 um 06:53 schrieb Jason Wang:
> 
>> This patch switches to use qemu_receive_packet() which can detect
>> reentrancy and return early.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>>   hw/net/dp8393x.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>> index 205c0decc5..019d4fe435 100644
>> --- a/hw/net/dp8393x.c
>> +++ b/hw/net/dp8393x.c
>> @@ -506,7 +506,7 @@ static void
>> dp8393x_do_transmit_packets(dp8393xState *s)
>>               s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>>               if (nc->info->can_receive(nc)) {
>>                   s->loopback_packet = 1;
>> -                nc->info->receive(nc, s->tx_buffer, tx_len);
>> +                qemu_receice_packet(nc, s->tx_buffer, tx_Len);
> 
> 
> Did you test compilation of that code? It looks like a typo ...

With typo fixed:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

[Stefan Weil]

Am 25.02.21 um 15:36 schrieb Philippe Mathieu-Daudé:

> On 2/24/21 7:13 AM, Stefan Weil wrote:
>> Am 24.02.21 um 06:53 schrieb Jason Wang:
>>
>>> This patch switches to use qemu_receive_packet() which can detect
>>> reentrancy and return early.
>>>
>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> ---
>>>    hw/net/dp8393x.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>> index 205c0decc5..019d4fe435 100644
>>> --- a/hw/net/dp8393x.c
>>> +++ b/hw/net/dp8393x.c
>>> @@ -506,7 +506,7 @@ static void
>>> dp8393x_do_transmit_packets(dp8393xState *s)
>>>                s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>>>                if (nc->info->can_receive(nc)) {
>>>                    s->loopback_packet = 1;
>>> -                nc->info->receive(nc, s->tx_buffer, tx_len);
>>> +                qemu_receice_packet(nc, s->tx_buffer, tx_Len);
>>
>> Did you test compilation of that code? It looks like a typo ...
> With typo fixed:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>


Plural, as I noticed later: there are two typos in the same line of 
code. The compiler will report both of them.

Stefan

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Alexander Bulekov]

On 210225 1931, P J P wrote:
> +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+
> | On 2/24/21 2:17 PM, Jason Wang wrote:
> | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
> | >> IIUC the guest could trigger an infinite loop and brick the emulated 
> | >> device model. Likely exhausting the stack, so either SEGV by corruption 
> | >> or some ENOMEM?
> | > 
> | > Yes.
> | >>
> | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list 
> | >> and ask for a CVE for this issue, so distributions can track the patches 
> | >> to backport in their stable releases? (it seems to be within the KVM 
> | >> devices boundary).
> | > 
> | > 
> | > That's the plan. I discussed this with Prasad before and he promise to
> | > ask CVE for this.
> 
> 'CVE-2021-3416' is assigned to this issue by Red Hat Inc.
> 

Hi Prasad,
What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't
those just manifestations of this bug for the e1000 and the eepro100
bug?
-Alex

> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D

Re: [PATCH 1/6] net: introduce qemu_receive_packet()

[Alexander Bulekov]

On 210225 1128, Alexander Bulekov wrote:
> On 210225 1931, P J P wrote:
> > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+
> > | On 2/24/21 2:17 PM, Jason Wang wrote:
> > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
> > | >> IIUC the guest could trigger an infinite loop and brick the emulated 
> > | >> device model. Likely exhausting the stack, so either SEGV by corruption 
> > | >> or some ENOMEM?
> > | > 
> > | > Yes.
> > | >>
> > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list 
> > | >> and ask for a CVE for this issue, so distributions can track the patches 
> > | >> to backport in their stable releases? (it seems to be within the KVM 
> > | >> devices boundary).
> > | > 
> > | > 
> > | > That's the plan. I discussed this with Prasad before and he promise to
> > | > ask CVE for this.
> > 
> > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc.
> > 
> 
> Hi Prasad,
> What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't
> those just manifestations of this bug for the e1000 and the eepro100
> bug?
^^ *devices

links:
https://www.openwall.com/lists/oss-security/2021/02/25/1
https://www.openwall.com/lists/oss-security/2021/02/25/2

> -Alex
> 
> > Thank you.
> > --
> > Prasad J Pandit / Red Hat Product Security Team
> > 8685 545E B54C 486B C6EB 271E E285 8B5A F050 DE8D
> 

Re: [PATCH 3/6] dp8393x: switch to use qemu_receive_packet() for loopback packet

[Jason Wang]

On 2021/2/25 10:42 下午, Stefan Weil wrote:
> Am 25.02.21 um 15:36 schrieb Philippe Mathieu-Daudé:
>
>> On 2/24/21 7:13 AM, Stefan Weil wrote:
>>> Am 24.02.21 um 06:53 schrieb Jason Wang:
>>>
>>>> This patch switches to use qemu_receive_packet() which can detect
>>>> reentrancy and return early.
>>>>
>>>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>>>> ---
>>>>    hw/net/dp8393x.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/net/dp8393x.c b/hw/net/dp8393x.c
>>>> index 205c0decc5..019d4fe435 100644
>>>> --- a/hw/net/dp8393x.c
>>>> +++ b/hw/net/dp8393x.c
>>>> @@ -506,7 +506,7 @@ static void
>>>> dp8393x_do_transmit_packets(dp8393xState *s)
>>>>                s->regs[SONIC_TCR] |= SONIC_TCR_CRSL;
>>>>                if (nc->info->can_receive(nc)) {
>>>>                    s->loopback_packet = 1;
>>>> -                nc->info->receive(nc, s->tx_buffer, tx_len);
>>>> +                qemu_receice_packet(nc, s->tx_buffer, tx_Len);
>>>
>>> Did you test compilation of that code? It looks like a typo ...
>> With typo fixed:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>
>
> Plural, as I noticed later: there are two typos in the same line of 
> code. The compiler will report both of them.
>
> Stefan


Yes, my bad. Will post V2.

Thanks

Re: [QEMU-SECURITY] [PATCH 1/6] net: introduce qemu_receive_packet()

[P J P]

Hello Alex,

On Thursday, 25 February, 2021, 10:00:33 pm IST, Alexander Bulekov <alxndr@bu.edu> wrote: 
On 210225 1128, Alexander Bulekov wrote:
> On 210225 1931, P J P wrote:
> > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+
> > | On 2/24/21 2:17 PM, Jason Wang wrote:
> > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
> > | >> IIUC the guest could trigger an infinite loop and brick the emulated 
> > | >> device model. Likely exhausting the stack, so either SEGV by corruption 
> > | >> or some ENOMEM?
> > | > 
> > | > Yes.
> > | >>
> > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list 
> > | >> and ask for a CVE for this issue, so distributions can track the patches 
> > | >> to backport in their stable releases? (it seems to be within the KVM 
> > | >> devices boundary).
> > | > 
> > | > 
> > | > That's the plan. I discussed this with Prasad before and he promise to
> > | > ask CVE for this.
> > 
> > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc.
>
> What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't
> those just manifestations of this bug for the e1000 and the eepro100
> devices

* You mean manifestations of the dam re-entrancy issue? 

* They have separate CVEs because they are fixed individually.


Thank you.
---
  -P J P
http://feedmug.com

[PATCH] rtl8193: switch to use qemu_receive_packet() for loopback

[Alexander Bulekov]

This patch switches to use qemu_receive_packet() which can detect
reentrancy and return early.

Buglink: https://bugs.launchpad.net/qemu/+bug/1910826
Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---

Although it's not a nc->info->receive() call, maybe this can also go in
this series?

-Alex

 hw/net/rtl8139.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 4675ac878e..90b4fc63ce 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -1795,7 +1795,7 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
         }
 
         DPRINTF("+++ transmit loopback mode\n");
-        rtl8139_do_receive(qemu_get_queue(s->nic), buf, size, do_interrupt);
+        qemu_receive_packet(qemu_get_queue(s->nic), buf, size);
 
         if (iov) {
             g_free(buf2);
-- 
2.28.0

Re: [QEMU-SECURITY] [PATCH 1/6] net: introduce qemu_receive_packet()

[Alexander Bulekov]

On 210226 1814, P J P wrote:
> Hello Alex,
> 
> On Thursday, 25 February, 2021, 10:00:33 pm IST, Alexander Bulekov <alxndr@bu.edu> wrote: 
> On 210225 1128, Alexander Bulekov wrote:
> > On 210225 1931, P J P wrote:
> > > +-- On Wed, 24 Feb 2021, Philippe Mathieu-Daudé wrote --+
> > > | On 2/24/21 2:17 PM, Jason Wang wrote:
> > > | > On 2021/2/24 6:11 下午, Philippe Mathieu-Daudé wrote:
> > > | >> IIUC the guest could trigger an infinite loop and brick the emulated 
> > > | >> device model. Likely exhausting the stack, so either SEGV by corruption 
> > > | >> or some ENOMEM?
> > > | > 
> > > | > Yes.
> > > | >>
> > > | >> Since this is guest triggerable, shouldn't we contact qemu-security@ list 
> > > | >> and ask for a CVE for this issue, so distributions can track the patches 
> > > | >> to backport in their stable releases? (it seems to be within the KVM 
> > > | >> devices boundary).
> > > | > 
> > > | > 
> > > | > That's the plan. I discussed this with Prasad before and he promise to
> > > | > ask CVE for this.
> > > 
> > > 'CVE-2021-3416' is assigned to this issue by Red Hat Inc.
> >
> > What is the difference with CVE-2021-20255 and CVE-2021-20257 ? Aren't
> > those just manifestations of this bug for the e1000 and the eepro100
> > devices
> 
> * You mean manifestations of the dam re-entrancy issue? 
> 

Ah I got confused - those other CVEs don't seem to be related to
loopback.
-Alex

> * They have separate CVEs because they are fixed individually.
> 
> 
> Thank you.
> ---
>   -P J P
> http://feedmug.com

Re: [PATCH] rtl8193: switch to use qemu_receive_packet() for loopback

[Jason Wang]

On 2021/2/27 2:47 上午, Alexander Bulekov wrote:
> This patch switches to use qemu_receive_packet() which can detect
> reentrancy and return early.
>
> Buglink: https://bugs.launchpad.net/qemu/+bug/1910826
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
>
> Although it's not a nc->info->receive() call, maybe this can also go in
> this series?
>
> -Alex


Yes, I will add this in this series.

Thanks


>
>   hw/net/rtl8139.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
> index 4675ac878e..90b4fc63ce 100644
> --- a/hw/net/rtl8139.c
> +++ b/hw/net/rtl8139.c
> @@ -1795,7 +1795,7 @@ static void rtl8139_transfer_frame(RTL8139State *s, uint8_t *buf, int size,
>           }
>   
>           DPRINTF("+++ transmit loopback mode\n");
> -        rtl8139_do_receive(qemu_get_queue(s->nic), buf, size, do_interrupt);
> +        qemu_receive_packet(qemu_get_queue(s->nic), buf, size);
>   
>           if (iov) {
>               g_free(buf2);