From 024e0e9095823b585fd2e66d1be20808a01fc0b1 Mon Sep 17 00:00:00 2001 From: Dariusz Stojaczyk Date: Mon, 9 Oct 2017 17:28:00 +0200 Subject: [PATCH] vhost: check against virtio descriptor table overflow Also squashed function has_next_desc into get_next_desc to simplify the code. We can't just mask indexes with (desc_table_size - 1), since in indirect descriptors case desc_table_size might not be a power of 2. Change-Id: I8053b0e37c553548d76c7a9cfe6b4dbc11c28cfc Signed-off-by: Dariusz Stojaczyk Reviewed-on: https://review.gerrithub.io/373744 Tested-by: SPDK Automated Test System Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp --- lib/vhost/vhost.c | 43 ++++-- lib/vhost/vhost_blk.c | 20 ++- lib/vhost/vhost_internal.h | 37 ++++- lib/vhost/vhost_scsi.c | 140 +++++++++++------- .../unit/lib/vhost/vhost_blk.c/vhost_blk_ut.c | 9 +- .../lib/vhost/vhost_scsi.c/vhost_scsi_ut.c | 8 +- 6 files changed, 173 insertions(+), 84 deletions(-) diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c index 5b9672932..bcd4db2cd 100644 --- a/lib/vhost/vhost.c +++ b/lib/vhost/vhost.c @@ -114,11 +114,19 @@ spdk_vhost_vq_avail_ring_get(struct rte_vhost_vring *vq, uint16_t *reqs, uint16_ return count; } -struct vring_desc * -spdk_vhost_vq_get_desc(struct rte_vhost_vring *vq, uint16_t req_idx) +int +spdk_vhost_vq_get_desc(struct rte_vhost_vring *vq, uint16_t req_idx, struct vring_desc **desc, + struct vring_desc **desc_table, uint32_t *desc_table_size) { - assert(req_idx < vq->size); - return &vq->desc[req_idx]; + if (spdk_unlikely(req_idx >= vq->size)) { + return -1; + } + + *desc = &vq->desc[req_idx]; + *desc_table = vq->desc; + *desc_table_size = vq->size; + + return 0; } /* @@ -156,17 +164,26 @@ spdk_vhost_vq_used_ring_enqueue(struct spdk_vhost_dev *vdev, struct rte_vhost_vr } } -bool -spdk_vhost_vring_desc_has_next(struct vring_desc *cur_desc) +int +spdk_vhost_vring_desc_get_next(struct vring_desc **desc, + struct vring_desc *desc_table, uint32_t desc_table_size) { - return !!(cur_desc->flags & VRING_DESC_F_NEXT); -} + struct vring_desc *old_desc = *desc; + uint16_t next_idx; -struct vring_desc * -spdk_vhost_vring_desc_get_next(struct vring_desc *vq_desc, struct vring_desc *cur_desc) -{ - assert(spdk_vhost_vring_desc_has_next(cur_desc)); - return &vq_desc[cur_desc->next]; + if ((old_desc->flags & VRING_DESC_F_NEXT) == 0) { + *desc = NULL; + return 0; + } + + next_idx = old_desc->next; + if (spdk_unlikely(next_idx >= desc_table_size)) { + *desc = NULL; + return -1; + } + + *desc = &desc_table[next_idx]; + return 0; } bool diff --git a/lib/vhost/vhost_blk.c b/lib/vhost/vhost_blk.c index a9dd2c1b4..a33709b85 100644 --- a/lib/vhost/vhost_blk.c +++ b/lib/vhost/vhost_blk.c @@ -125,9 +125,16 @@ static int blk_iovs_setup(struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq, uint16_t req_idx, struct iovec *iovs, uint16_t *iovs_cnt, uint32_t *length) { - struct vring_desc *desc = spdk_vhost_vq_get_desc(vq, req_idx); + struct vring_desc *desc, *desc_table; uint16_t out_cnt = 0, cnt = 0; - uint32_t len = 0; + uint32_t desc_table_size, len = 0; + int rc; + + rc = spdk_vhost_vq_get_desc(vq, req_idx, &desc, &desc_table, &desc_table_size); + if (rc != 0) { + SPDK_ERRLOG("%s: Invalid descriptor at index %"PRIu16".\n", vdev->name, req_idx); + return -1; + } while (1) { /* @@ -150,9 +157,12 @@ blk_iovs_setup(struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq, uint16_t out_cnt += spdk_vhost_vring_desc_is_wr(desc); - if (spdk_vhost_vring_desc_has_next(desc)) { - desc = spdk_vhost_vring_desc_get_next(vq->desc, desc); - } else { + rc = spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_size); + if (rc != 0) { + SPDK_ERRLOG("%s: Descriptor chain at index %"PRIu16" terminated unexpectedly.\n", + vdev->name, req_idx); + return -1; + } else if (desc == NULL) { break; } } diff --git a/lib/vhost/vhost_internal.h b/lib/vhost/vhost_internal.h index cffb8f597..cd4d220bb 100644 --- a/lib/vhost/vhost_internal.h +++ b/lib/vhost/vhost_internal.h @@ -128,13 +128,40 @@ uint16_t spdk_vhost_vq_avail_ring_get(struct rte_vhost_vring *vq, uint16_t *reqs uint16_t reqs_len); bool spdk_vhost_vq_should_notify(struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq); -struct vring_desc *spdk_vhost_vq_get_desc(struct rte_vhost_vring *vq, uint16_t req_idx); - +/** + * Get a virtio descriptor at given index in given virtqueue. + * The descriptor will provide access to the entire descriptor + * chain. The subsequent descriptors are accesible via + * \c spdk_vhost_vring_desc_get_next. + * \param vq virtqueue + * \param req_idx descriptor index + * \param desc pointer to be set to the descriptor + * \param desc_table descriptor table to be used with + * \c spdk_vhost_vring_desc_get_next. This might be either + * default virtqueue descriptor table or per-chain indirect + * table. + * \param desc_table_size size of the *desc_table* + * \return 0 on success, -1 if given index is invalid. + * If -1 is returned, the params won't be changed. + */ +int spdk_vhost_vq_get_desc(struct rte_vhost_vring *vq, uint16_t req_idx, struct vring_desc **desc, + struct vring_desc **desc_table, uint32_t *desc_table_size); void spdk_vhost_vq_used_ring_enqueue(struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq, uint16_t id, uint32_t len); -bool spdk_vhost_vring_desc_has_next(struct vring_desc *cur_desc); -struct vring_desc *spdk_vhost_vring_desc_get_next(struct vring_desc *vq_desc, - struct vring_desc *cur_desc); + +/** + * Get subsequent descriptor from given table. + * \param desc current descriptor, will be set to the + * next descriptor (NULL in case this is the last + * descriptor in the chain or the next desc is invalid) + * \param desc_table descriptor table + * \param desc_table_size size of the *desc_table* + * \return 0 on success, -1 if given index is invalid + * The *desc* param will be set regardless of the + * return value. + */ +int spdk_vhost_vring_desc_get_next(struct vring_desc **desc, + struct vring_desc *desc_table, uint32_t desc_table_size); bool spdk_vhost_vring_desc_is_wr(struct vring_desc *cur_desc); int spdk_vhost_vring_desc_to_iov(struct spdk_vhost_dev *vdev, struct iovec *iov, diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index 8d9215f21..1139677a8 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -174,10 +174,11 @@ eventq_enqueue(struct spdk_vhost_scsi_dev *svdev, unsigned scsi_dev_num, uint32_ uint32_t reason) { struct rte_vhost_vring *vq; - struct vring_desc *desc; + struct vring_desc *desc, *desc_table; struct virtio_scsi_event *desc_ev; - uint32_t req_size; + uint32_t desc_table_size, req_size = 0; uint16_t req; + int rc; assert(scsi_dev_num < SPDK_VHOST_SCSI_CTRLR_MAX_DEVS); @@ -189,29 +190,37 @@ eventq_enqueue(struct spdk_vhost_scsi_dev *svdev, unsigned scsi_dev_num, uint32_ return; } - desc = spdk_vhost_vq_get_desc(vq, req); - desc_ev = spdk_vhost_gpa_to_vva(&svdev->vdev, desc->addr); - - if (desc->len < sizeof(*desc_ev) || desc_ev == NULL) { - SPDK_ERRLOG("Controller %s: Invalid eventq descriptor.\n", svdev->vdev.name); - req_size = 0; - } else { - desc_ev->event = event; - desc_ev->lun[0] = 1; - desc_ev->lun[1] = scsi_dev_num; - /* virtio LUN id 0 can refer either to the entire device - * or actual LUN 0 (the only supported by vhost for now) - */ - desc_ev->lun[2] = 0 >> 8; - desc_ev->lun[3] = 0 & 0xFF; - /* virtio doesn't specify any strict format for LUN id (bytes 2 and 3) - * current implementation relies on linux kernel sources - */ - memset(&desc_ev->lun[4], 0, 4); - desc_ev->reason = reason; - req_size = sizeof(*desc_ev); + rc = spdk_vhost_vq_get_desc(vq, req, &desc, &desc_table, &desc_table_size); + if (rc != 0 || desc->len < sizeof(*desc_ev)) { + SPDK_ERRLOG("Controller %s: Invalid eventq descriptor at index %"PRIu16".\n", + svdev->vdev.name, req); + goto out; } + desc_ev = spdk_vhost_gpa_to_vva(&svdev->vdev, desc->addr); + if (desc->len < sizeof(*desc_ev) || desc_ev == NULL) { + SPDK_ERRLOG("Controller %s: Invalid eventq descriptor at index %"PRIu16".\n", + svdev->vdev.name, req); + req_size = 0; + goto out; + } + + desc_ev->event = event; + desc_ev->lun[0] = 1; + desc_ev->lun[1] = scsi_dev_num; + /* virtio LUN id 0 can refer either to the entire device + * or actual LUN 0 (the only supported by vhost for now) + */ + desc_ev->lun[2] = 0 >> 8; + desc_ev->lun[3] = 0 & 0xFF; + /* virtio doesn't specify any strict format for LUN id (bytes 2 and 3) + * current implementation relies on linux kernel sources + */ + memset(&desc_ev->lun[4], 0, 4); + desc_ev->reason = reason; + req_size = sizeof(*desc_ev); + +out: spdk_vhost_vq_used_ring_enqueue(&svdev->vdev, vq, req, req_size); } @@ -305,30 +314,42 @@ spdk_vhost_scsi_task_init_target(struct spdk_vhost_scsi_task *task, const __u8 * static void process_ctrl_request(struct spdk_vhost_scsi_task *task) { - struct vring_desc *desc; + struct vring_desc *desc, *desc_table; struct virtio_scsi_ctrl_tmf_req *ctrl_req; struct virtio_scsi_ctrl_an_resp *an_resp; + uint32_t desc_table_size; + int rc; spdk_scsi_task_construct(&task->scsi, spdk_vhost_scsi_task_mgmt_cpl, spdk_vhost_scsi_task_free_cb, NULL); - desc = spdk_vhost_vq_get_desc(task->vq, task->req_idx); + rc = spdk_vhost_vq_get_desc(task->vq, task->req_idx, &desc, &desc_table, &desc_table_size); + if (spdk_unlikely(rc != 0)) { + SPDK_ERRLOG("%s: Invalid controlq descriptor at index %d.\n", + task->svdev->vdev.name, task->req_idx); + goto out; + } + ctrl_req = spdk_vhost_gpa_to_vva(&task->svdev->vdev, desc->addr); SPDK_DEBUGLOG(SPDK_TRACE_VHOST_SCSI_QUEUE, "Processing controlq descriptor: desc %d/%p, desc_addr %p, len %d, flags %d, last_used_idx %d; kickfd %d; size %d\n", task->req_idx, desc, (void *)desc->addr, desc->len, desc->flags, task->vq->last_used_idx, task->vq->kickfd, task->vq->size); - SPDK_TRACEDUMP(SPDK_TRACE_VHOST_SCSI_QUEUE, "Request desriptor", (uint8_t *)ctrl_req, + SPDK_TRACEDUMP(SPDK_TRACE_VHOST_SCSI_QUEUE, "Request descriptor", (uint8_t *)ctrl_req, desc->len); spdk_vhost_scsi_task_init_target(task, ctrl_req->lun); + spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_size); + if (spdk_unlikely(desc == NULL)) { + SPDK_ERRLOG("%s: No response descriptor for controlq request %d.\n", + task->svdev->vdev.name, task->req_idx); + goto out; + } + /* Process the TMF request */ switch (ctrl_req->type) { case VIRTIO_SCSI_T_TMF: - /* Get the response buffer */ - assert(spdk_vhost_vring_desc_has_next(desc)); - desc = spdk_vhost_vring_desc_get_next(task->vq->desc, desc); task->tmf_resp = spdk_vhost_gpa_to_vva(&task->svdev->vdev, desc->addr); /* Check if we are processing a valid request */ @@ -353,7 +374,6 @@ process_ctrl_request(struct spdk_vhost_scsi_task *task) break; case VIRTIO_SCSI_T_AN_QUERY: case VIRTIO_SCSI_T_AN_SUBSCRIBE: { - desc = spdk_vhost_vring_desc_get_next(task->vq->desc, desc); an_resp = spdk_vhost_gpa_to_vva(&task->svdev->vdev, desc->addr); an_resp->response = VIRTIO_SCSI_S_ABORTED; break; @@ -363,6 +383,7 @@ process_ctrl_request(struct spdk_vhost_scsi_task *task) break; } +out: spdk_vhost_vq_used_ring_enqueue(&task->svdev->vdev, task->vq, task->req_idx, 0); spdk_vhost_scsi_task_put(task); } @@ -377,15 +398,16 @@ static int task_data_setup(struct spdk_vhost_scsi_task *task, struct virtio_scsi_cmd_req **req) { - struct rte_vhost_vring *vq = task->vq; struct spdk_vhost_dev *vdev = &task->svdev->vdev; - struct vring_desc *desc = spdk_vhost_vq_get_desc(task->vq, task->req_idx); + struct vring_desc *desc, *desc_table; struct iovec *iovs = task->iovs; uint16_t iovcnt = 0, iovcnt_max = SPDK_VHOST_IOVS_MAX; - uint32_t len = 0; + uint32_t desc_table_len, len = 0; + int rc; - /* Sanity check. First descriptor must be readable and must have next one. */ - if (spdk_unlikely(spdk_vhost_vring_desc_is_wr(desc) || !spdk_vhost_vring_desc_has_next(desc))) { + rc = spdk_vhost_vq_get_desc(task->vq, task->req_idx, &desc, &desc_table, &desc_table_len); + /* First descriptor must be readable */ + if (rc != 0 || spdk_unlikely(spdk_vhost_vring_desc_is_wr(desc))) { SPDK_WARNLOG("Invalid first (request) descriptor.\n"); task->resp = NULL; goto abort_task; @@ -394,7 +416,14 @@ task_data_setup(struct spdk_vhost_scsi_task *task, spdk_scsi_task_construct(&task->scsi, spdk_vhost_scsi_task_cpl, spdk_vhost_scsi_task_free_cb, NULL); *req = spdk_vhost_gpa_to_vva(vdev, desc->addr); - desc = spdk_vhost_vring_desc_get_next(vq->desc, desc); + /* Each request must have at least 2 descriptors (e.g. request and response) */ + spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_len); + if (desc == NULL) { + SPDK_WARNLOG("%s: Descriptor chain at index %d contains neither payload nor response buffer.\n", + vdev->name, task->req_idx); + task->resp = NULL; + goto abort_task; + } task->scsi.dxfer_dir = spdk_vhost_vring_desc_is_wr(desc) ? SPDK_SCSI_DIR_FROM_DEV : SPDK_SCSI_DIR_TO_DEV; task->scsi.iovs = iovs; @@ -404,7 +433,16 @@ task_data_setup(struct spdk_vhost_scsi_task *task, * FROM_DEV (READ): [RD_req][WR_resp][WR_buf0]...[WR_bufN] */ task->resp = spdk_vhost_gpa_to_vva(vdev, desc->addr); - if (!spdk_vhost_vring_desc_has_next(desc)) { + + rc = spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_len); + if (spdk_unlikely(rc != 0)) { + SPDK_WARNLOG("%s: invalid descriptor chain at request index %d (descriptor id overflow?).\n", + vdev->name, task->req_idx); + task->resp = NULL; + goto abort_task; + } + + if (desc == NULL) { /* * TEST UNIT READY command and some others might not contain any payload and this is not an error. */ @@ -418,22 +456,24 @@ task_data_setup(struct spdk_vhost_scsi_task *task, return 0; } - desc = spdk_vhost_vring_desc_get_next(vq->desc, desc); - /* All remaining descriptors are data. */ - while (iovcnt < iovcnt_max) { + while (desc && iovcnt < iovcnt_max) { + if (spdk_unlikely(!spdk_vhost_vring_desc_is_wr(desc))) { + SPDK_WARNLOG("FROM DEV cmd: descriptor nr %" PRIu16" in payload chain is read only.\n", iovcnt); + task->resp = NULL; + goto abort_task; + } + if (spdk_unlikely(spdk_vhost_vring_desc_to_iov(vdev, iovs, &iovcnt, desc))) { task->resp = NULL; goto abort_task; } len += desc->len; - if (!spdk_vhost_vring_desc_has_next(desc)) - break; - - desc = spdk_vhost_vring_desc_get_next(vq->desc, desc); - if (spdk_unlikely(!spdk_vhost_vring_desc_is_wr(desc))) { - SPDK_WARNLOG("FROM DEV cmd: descriptor nr %" PRIu16" in payload chain is read only.\n", iovcnt); + rc = spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_len); + if (spdk_unlikely(rc != 0)) { + SPDK_WARNLOG("%s: invalid payload in descriptor chain starting at index %d.\n", + vdev->name, task->req_idx); task->resp = NULL; goto abort_task; } @@ -453,19 +493,15 @@ task_data_setup(struct spdk_vhost_scsi_task *task, } len += desc->len; - if (!spdk_vhost_vring_desc_has_next(desc)) { + spdk_vhost_vring_desc_get_next(&desc, desc_table, desc_table_len); + if (spdk_unlikely(desc == NULL)) { SPDK_WARNLOG("TO_DEV cmd: no response descriptor.\n"); task->resp = NULL; goto abort_task; } - - desc = spdk_vhost_vring_desc_get_next(vq->desc, desc); } task->resp = spdk_vhost_gpa_to_vva(vdev, desc->addr); - if (spdk_vhost_vring_desc_has_next(desc)) { - SPDK_WARNLOG("TO_DEV cmd: ignoring unexpected descriptors after response descriptor.\n"); - } } if (iovcnt == iovcnt_max) { diff --git a/test/unit/lib/vhost/vhost_blk.c/vhost_blk_ut.c b/test/unit/lib/vhost/vhost_blk.c/vhost_blk_ut.c index 7eec37d04..fbbb1de34 100644 --- a/test/unit/lib/vhost/vhost_blk.c/vhost_blk_ut.c +++ b/test/unit/lib/vhost/vhost_blk.c/vhost_blk_ut.c @@ -63,14 +63,13 @@ DEFINE_STUB(spdk_ring_enqueue, size_t, (struct spdk_ring *ring, void **objs, siz DEFINE_STUB(spdk_ring_dequeue, size_t, (struct spdk_ring *ring, void **objs, size_t count), 0); DEFINE_STUB_V(spdk_vhost_vq_used_ring_enqueue, (struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq, uint16_t id, uint32_t len)); -DEFINE_STUB_P(spdk_vhost_vq_get_desc, struct vring_desc, (struct rte_vhost_vring *vq, - uint16_t req_idx), {0}); +DEFINE_STUB(spdk_vhost_vq_get_desc, int, (struct rte_vhost_vring *vq, uint16_t req_idx, + struct vring_desc **desc, struct vring_desc **desc_table, uint32_t *desc_table_size), 0); DEFINE_STUB(spdk_vhost_vring_desc_is_wr, bool, (struct vring_desc *cur_desc), false); DEFINE_STUB(spdk_vhost_vring_desc_to_iov, int, (struct spdk_vhost_dev *vdev, struct iovec *iov, uint16_t *iov_index, const struct vring_desc *desc), 0); -DEFINE_STUB(spdk_vhost_vring_desc_has_next, bool, (struct vring_desc *cur_desc), false); -DEFINE_STUB_P(spdk_vhost_vring_desc_get_next, struct vring_desc, (struct vring_desc *vq_desc, - struct vring_desc *cur_desc), {0}); +DEFINE_STUB(spdk_vhost_vring_desc_get_next, int, (struct vring_desc **desc, + struct vring_desc *desc_table, uint32_t desc_table_size), 0); DEFINE_STUB(spdk_bdev_free_io, int, (struct spdk_bdev_io *bdev_io), 0); DEFINE_STUB(spdk_bdev_readv, int, (struct spdk_bdev_desc *desc, struct spdk_io_channel *ch, struct iovec *iov, int iovcnt, diff --git a/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c b/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c index beebd1427..0c51687b2 100644 --- a/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c +++ b/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c @@ -59,8 +59,8 @@ DEFINE_STUB_P(spdk_scsi_lun_get_name, const char, (const struct spdk_scsi_lun *l DEFINE_STUB(spdk_scsi_lun_get_id, int, (const struct spdk_scsi_lun *lun), 0); DEFINE_STUB(spdk_vhost_vq_avail_ring_get, uint16_t, (struct rte_vhost_vring *vq, uint16_t *reqs, uint16_t reqs_len), 0); -DEFINE_STUB_P(spdk_vhost_vq_get_desc, struct vring_desc, (struct rte_vhost_vring *vq, - uint16_t req_idx), {0}); +DEFINE_STUB(spdk_vhost_vq_get_desc, int, (struct rte_vhost_vring *vq, uint16_t req_idx, + struct vring_desc **desc, struct vring_desc **desc_table, uint32_t *desc_table_size), 0); DEFINE_STUB_VP(spdk_vhost_gpa_to_vva, (struct spdk_vhost_dev *vdev, uint64_t addr), {0}); DEFINE_STUB_V(spdk_vhost_vq_used_ring_enqueue, (struct spdk_vhost_dev *vdev, struct rte_vhost_vring *vq, uint16_t id, uint32_t len)); @@ -75,8 +75,8 @@ DEFINE_STUB_P(spdk_scsi_dev_find_port_by_id, struct spdk_scsi_port, (struct spdk DEFINE_STUB_V(spdk_scsi_task_construct, (struct spdk_scsi_task *task, spdk_scsi_task_cpl cpl_fn, spdk_scsi_task_free free_fn, struct spdk_scsi_task *parent)); DEFINE_STUB(spdk_vhost_vring_desc_has_next, bool, (struct vring_desc *cur_desc), false); -DEFINE_STUB_P(spdk_vhost_vring_desc_get_next, struct vring_desc, (struct vring_desc *vq_desc, - struct vring_desc *cur_desc), {0}); +DEFINE_STUB(spdk_vhost_vring_desc_get_next, int, (struct vring_desc **desc, + struct vring_desc *desc_table, uint32_t desc_table_size), 0); DEFINE_STUB_P(spdk_scsi_dev_get_lun, struct spdk_scsi_lun, (struct spdk_scsi_dev *dev, int lun_id), {0}); DEFINE_STUB(spdk_vhost_vring_desc_is_wr, bool, (struct vring_desc *cur_desc), false); DEFINE_STUB(spdk_vhost_vring_desc_to_iov, int, (struct spdk_vhost_dev *vdev, struct iovec *iov,