vhost: fix crash when exceeding file descriptors

Protect against DPDK crash when allocation of listen fd >= 1023.
For events on fd:s >1023, the current implementation will trigger
an abort due to access outside of allocated bit mask.

Corrections would include:

  * Match fdset_add() signature in fd_man.c to fd_man.h
  * Handling of return codes from fdset_add()
  * Addition of check of fd number in fdset_add_fd()

The rationale behind the suggested code change is that,
fdset_event_dispatch() could attempt access outside of the FD_SET
bitmask if there is an event on a file descriptor that in turn
looks up a virtio file descriptor with a value > 1023.
Such an attempt will lead to an abort() and a restart of any
vswitch using DPDK.

A discussion topic exist in the ovs-discuss mailing list that can
provide a little more background:
http://openvswitch.org/pipermail/discuss/2016-February/020243.html

Fixes: 8f972312 ("vhost: support vhost-user")

Signed-off-by: Patrik Andersson <patrik.r.andersson@ericsson.com>
Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

diff --git a/lib/librte_vhost/vhost_user/fd_man.c b/lib/librte_vhost/vhost_user/fd_man.c
index 087aaed..c691339 100644 (file)
--- a/lib/librte_vhost/vhost_user/fd_man.c
+++ b/lib/librte_vhost/vhost_user/fd_man.c
@@ -71,20 +71,22 @@ fdset_find_free_slot(struct fdset *pfdset)
        return fdset_find_fd(pfdset, -1);
 }
 
-static void
+static int
 fdset_add_fd(struct fdset  *pfdset, int idx, int fd,
        fd_cb rcb, fd_cb wcb, void *dat)
 {
        struct fdentry *pfdentry;
 
-       if (pfdset == NULL || idx >= MAX_FDS)
-               return;
+       if (pfdset == NULL || idx >= MAX_FDS || fd >= FD_SETSIZE)
+               return -1;
 
        pfdentry = &pfdset->fd[idx];
        pfdentry->fd = fd;
        pfdentry->rcb = rcb;
        pfdentry->wcb = wcb;
        pfdentry->dat = dat;
+
+       return 0;
 }
 
 /**
@@ -150,12 +152,11 @@ fdset_add(struct fdset *pfdset, int fd, fd_cb rcb, fd_cb wcb, void *dat)
 
        /* Find a free slot in the list. */
        i = fdset_find_free_slot(pfdset);
-       if (i == -1) {
+       if (i == -1 || fdset_add_fd(pfdset, i, fd, rcb, wcb, dat) < 0) {
                pthread_mutex_unlock(&pfdset->fd_mutex);
                return -2;
        }
 
-       fdset_add_fd(pfdset, i, fd, rcb, wcb, dat);
        pfdset->num++;
 
        pthread_mutex_unlock(&pfdset->fd_mutex);

diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c
index a0d83f3..f0ea3a2 100644 (file)
--- a/lib/librte_vhost/vhost_user/vhost-net-user.c
+++ b/lib/librte_vhost/vhost_user/vhost-net-user.c
@@ -257,6 +257,7 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
        int vid;
        size_t size;
        struct vhost_user_connection *conn;
+       int ret;
 
        conn = malloc(sizeof(*conn));
        if (conn == NULL) {
@@ -278,7 +279,15 @@ vhost_user_add_connection(int fd, struct vhost_user_socket *vsocket)
 
        conn->vsocket = vsocket;
        conn->vid = vid;
-       fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler, NULL, conn);
+       ret = fdset_add(&vhost_user.fdset, fd, vhost_user_msg_handler,
+                       NULL, conn);
+       if (ret < 0) {
+               free(conn);
+               close(fd);
+               RTE_LOG(ERR, VHOST_CONFIG,
+                       "failed to add fd %d into vhost server fdset\n",
+                       fd);
+       }
 }
 
 /* call back when there is new vhost-user connection from client  */
@@ -470,8 +479,14 @@ vhost_user_create_server(struct vhost_user_socket *vsocket)
                goto err;
 
        vsocket->listenfd = fd;
-       fdset_add(&vhost_user.fdset, fd, vhost_user_server_new_connection,
+       ret = fdset_add(&vhost_user.fdset, fd, vhost_user_server_new_connection,
                  NULL, vsocket);
+       if (ret < 0) {
+               RTE_LOG(ERR, VHOST_CONFIG,
+                       "failed to add listen fd %d to vhost server fdset\n",
+                       fd);
+               goto err;
+       }
 
        return 0;