diff mbox series

[1/6] net: introduce qemu_receive_packet()

Message ID 20210224055401.492407-2-jasowang@redhat.com
State New
Headers show
Series Detect reentrant RX casue by loopback | expand

Commit Message

Jason Wang Feb. 24, 2021, 5:53 a.m. UTC
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(-)

Comments

Philippe Mathieu-Daudé Feb. 24, 2021, 10:11 a.m. UTC | #1
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(-)
Jason Wang Feb. 24, 2021, 1:17 p.m. UTC | #2
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(-)
Philippe Mathieu-Daudé Feb. 24, 2021, 1:43 p.m. UTC | #3
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
P J P Feb. 25, 2021, 2:01 p.m. UTC | #4
+-- 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
Philippe Mathieu-Daudé Feb. 25, 2021, 2:31 p.m. UTC | #5
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>
Alexander Bulekov Feb. 25, 2021, 4:28 p.m. UTC | #6
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
Alexander Bulekov Feb. 25, 2021, 4:29 p.m. UTC | #7
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
>
P J P Feb. 26, 2021, 6:14 p.m. UTC | #8
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
Alexander Bulekov Feb. 26, 2021, 6:53 p.m. UTC | #9
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
diff mbox series

Patch

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,