qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early


From: Jason Wang
Subject: Re: [Qemu-devel] [PATCH V4 for 3.1 1/4] net: drop too large packet early
Date: Tue, 4 Dec 2018 10:55:53 +0800
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1


On 2018/12/4 上午2:13, Thomas Huth wrote:
On 2018-12-03 11:06, Jason Wang wrote:
We try to detect and drop too large packet (>INT_MAX) in 1592a9947036
("net: ignore packet size greater than INT_MAX") during packet
delivering. Unfortunately, this is not sufficient as we may hit
another integer overflow when trying to queue such large packet in
qemu_net_queue_append_iov():

- size of the allocation may overflow on 32bit
- packet->size is integer which may overflow even on 64bit

Fixing this by move the check to qemu_sendv_packet_async() which is
the entrance of all networking codes and reduce the limit to
NET_BUFSIZE to be more conservative.

Cc: address@hidden
Cc: Li Qiang <address@hidden>
Reported-by: Li Qiang <address@hidden>
Reviewed-by: Li Qiang <address@hidden>
Signed-off-by: Jason Wang <address@hidden>
---
  net/net.c | 13 +++++++------
  1 file changed, 7 insertions(+), 6 deletions(-)
Since this is a critical patch for rc4, here's a verbose review...

diff --git a/net/net.c b/net/net.c
index 07c194a8f6..affe1877cf 100644
--- a/net/net.c
+++ b/net/net.c
@@ -712,15 +712,11 @@ ssize_t qemu_deliver_packet_iov(NetClientState *sender,
                                  void *opaque)
  {
      NetClientState *nc = opaque;
-    size_t size = iov_size(iov, iovcnt);
      int ret;
- if (size > INT_MAX) {
-        return size;
-    }
if (nc->link_down) {
-        return size;
+        return iov_size(iov, iovcnt);
      }
In case you respin this patch again, please make
qemu_deliver_packet_iov() "static", so that it is clear that it can not
be called directly from the outside anymore. And in case there is no
need to respin, please consider to send a separate patch for 4.0 instead.


Yes, will try to make it for the next version.



Ok, thinking now load about the call chain:

Anyway, qemu_deliver_packet_iov is not directly called from any other
file currently, so this is ok ...

So let's see how it is used in net.c ... it's only used as paramter
here: qemu_new_net_queue(qemu_deliver_packet_iov, nc) ...
qemu_new_net_queue() assigns it to NetQueue->deliver which is only used
within queue.c.

Functions using that ->deliver function pointer are the static functions
qemu_net_queue_deliver() and qemu_net_queue_deliver_iov(). First one
only uses one iov, so I don't think we can overflow the size here.
Second one is used in turn are used by the public function
qemu_net_queue_send_iov(). This has two callers, one in net.c in
qemu_sendv_packet_async() which you guard below ==> OK.
The other caller is in qemu_netfilter_pass_to_next() in filter.c - and
this function is called from many more other places ... but as far as I
can see, these don't call it in a way where the size could overflow.

==> Removing the check in qemu_deliver_packet_iov() sounds ok to me, if
it is checked in qemu_sendv_packet_async() instead.


Yes. Let me add more in the commit message.



      if (nc->receive_disabled) {
@@ -745,10 +741,15 @@ ssize_t qemu_sendv_packet_async(NetClientState *sender,
                                  NetPacketSent *sent_cb)
  {
      NetQueue *queue;
+    size_t size = iov_size(iov, iovcnt);
      int ret;
+ if (size > NET_BUFSIZE) {
+        return size;
+    }
It's a little bit unfortunate that the unsigned size will be cast to
ssize_t, so a very large size could suddenly change sign. But as Eric
already wrote in his mail, it seems like the callers are either ignoring
the return value, or just checking for != 0, so it should be ok for now.

To be more consistent, maybe it would be better to always return an
negative error code here instead?


Yes, and we can do it for 4.0.



      if (sender->link_down || !sender->peer) {
-        return iov_size(iov, iovcnt);
+        return size;
      }
/* Let filters handle the packet first */

I think I'm fine if this patch is applied in its current shape for rc4, so:

Reviewed-by: Thomas Huth <address@hidden>

... but please consider some follow-up patches to make
qemu_deliver_packet_iov() static, and maybe to return an error code in
qemu_sendv_packet_async() instead.


Yes.


Thanks for the reviewing.




reply via email to

[Prev in Thread] Current Thread [Next in Thread]