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

[Thomas Huth <thuth@redhat.com>]

On 2018-12-04 04:53, 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 moving 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. This works since:
> 
> - For the callers that call qemu_sendv_packet_async() directly, they
>   only care about if zero is returned to determine whether to prevent
>   the source from producing more packets. A callback will be triggered
>   if peer can accept more then source could be enabled. This is
>   usually used by high speed networking implementation like virtio-net
>   or netmap.
> - For the callers that call qemu_sendv_packet() that calls
>   qemu_sendv_packet_async() indirectly, they often ignore the return
>   value. In this case qemu will just the drop packets if peer can't
>   receive.
> 
> Qemu will copy the packet if it was queued. So it was safe for both
> kinds of the callers to assume the packet was sent.
> 
> Since we move the check from qemu_deliver_packet_iov() to
> qemu_sendv_packet_async(), it would be safer to make
> qemu_deliver_packet_iov() static to prevent any external user in the
> future.
> 
> This is a revised patch of CVE-2018-17963.
> 
> Cc: qemu-stable@nongnu.org
> Cc: Li Qiang <liq3ea@163.com>
> Fixes: 1592a9947036 ("net: ignore packet size greater than INT_MAX")
> Reported-by: Li Qiang <liq3ea@gmail.com>
> Reviewed-by: Li Qiang <liq3ea@gmail.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  include/net/net.h |  6 ------
>  net/net.c         | 28 +++++++++++++++++-----------
>  2 files changed, 17 insertions(+), 17 deletions(-)
> 
> diff --git a/include/net/net.h b/include/net/net.h
> index 7936d53d2f..ec13702dbf 100644
> --- a/include/net/net.h
> +++ b/include/net/net.h
> @@ -169,12 +169,6 @@ void qemu_check_nic_model(NICInfo *nd, const char *model);
>  int qemu_find_nic_model(NICInfo *nd, const char * const *models,
>                          const char *default_model);
>  
> -ssize_t qemu_deliver_packet_iov(NetClientState *sender,
> -                            unsigned flags,
> -                            const struct iovec *iov,
> -                            int iovcnt,
> -                            void *opaque);
> -
>  void print_net_client(Monitor *mon, NetClientState *nc);
>  void hmp_info_network(Monitor *mon, const QDict *qdict);
>  void net_socket_rs_init(SocketReadState *rs,
> diff --git a/net/net.c b/net/net.c
> index 07c194a8f6..1f7d626197 100644
> --- a/net/net.c
> +++ b/net/net.c
> @@ -231,6 +231,11 @@ static void qemu_net_client_destructor(NetClientState *nc)
>  {
>      g_free(nc);
>  }
> +static ssize_t qemu_deliver_packet_iov(NetClientState *sender,
> +                                       unsigned flags,
> +                                       const struct iovec *iov,
> +                                       int iovcnt,
> +                                       void *opaque);
>  
>  static void qemu_net_client_setup(NetClientState *nc,
>                                    NetClientInfo *info,
> @@ -705,22 +710,18 @@ static ssize_t nc_sendv_compat(NetClientState *nc, const struct iovec *iov,
>      return ret;
>  }
>  
> -ssize_t qemu_deliver_packet_iov(NetClientState *sender,
> -                                unsigned flags,
> -                                const struct iovec *iov,
> -                                int iovcnt,
> -                                void *opaque)
> +static ssize_t qemu_deliver_packet_iov(NetClientState *sender,
> +                                       unsigned flags,
> +                                       const struct iovec *iov,
> +                                       int iovcnt,
> +                                       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);
>      }
>  
>      if (nc->receive_disabled) {
> @@ -745,10 +746,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;
> +    }
> +
>      if (sender->link_down || !sender->peer) {
> -        return iov_size(iov, iovcnt);
> +        return size;
>      }
>  
>      /* Let filters handle the packet first */
> 

Reviewed-by: Thomas Huth <thuth@redhat.com>