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-sta...@nongnu.org
> Cc: Li Qiang <liq...@163.com>
> Fixes: 1592a9947036 ("net: ignore packet size greater than INT_MAX")
> Reported-by: Li Qiang <liq...@gmail.com>
> Reviewed-by: Li Qiang <liq...@gmail.com>
> Signed-off-by: Jason Wang <jasow...@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 <th...@redhat.com>
