rdma: various issues in rdma/pvrdma backend

From: Prasad J Pandit <[email protected]>

rdma back-end has scatter/gather array ibv_sge[MAX_SGE=4] set to have 4 elements. A guest could send a 'PvrdmaSqWqe' ring element with 'num_sge' set to > MAX_SGE, which may lead to OOB access issue. Add check to avoid it.

Reported-by Saar Amar <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/rdma_backend.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Update v1: use new error code VENDOR_ERR_INV_NUM_SGE
-> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02220.html

hw/rdma/rdma_backend.c +3 -3

index ae1e4dcb29..bd4710d16f 100644
@@ -476,9 +476,9 @@ void rdma_backend_post_send(RdmaBackendDev *backend_dev,
     }
 
     pr_dbg("num_sge=%d\n", num_sge);
-    if (!num_sge) {
-        pr_dbg("num_sge=0\n");
-        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_NO_SGE, ctx);
+    if (!num_sge || num_sge > MAX_SGE) {
+        pr_dbg("invalid num_sge=%d\n", num_sge);
+        complete_work(IBV_WC_GENERAL_ERR, VENDOR_ERR_INV_NUM_SGE, ctx);
         return;
     }
 

@@ -603,9 +603,9 @@ void rdma_backend_post_recv(RdmaBackendDev *backend_dev,
     }

Signature · 2 lines

-- 
2.19.2

From: Prasad J Pandit <[email protected]>

Define skeleton 'uar_read' routine. Avoid NULL dereference.

Reported-by Li Qiang <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
  hw/rdma/vmw/pvrdma_main.c | 6 ++++++
  1 file changed, 6 insertions(+)

hw/rdma/vmw/pvrdma_main.c +0 -0

index 23dc9926e3..8a03ab4669 100644

hw/rdma/vmw/pvrdma_main.c +3 -0

+}
+
  static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
  {
      PVRDMADev *dev = opaque;

@@ -489,6 +494,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
  }
 
  static const MemoryRegionOps uar_ops = {
+    .read = uar_read,
      .write = uar_write,
      .endianness = DEVICE_LITTLE_ENDIAN,
      .impl = {

From: Prasad J Pandit <[email protected]>

When creating CQ/QP rings, an object can have up to PVRDMA_MAX_FAST_REG_PAGES=128 pages. Check 'npages' parameter to avoid excessive memory allocation or a null dereference.

Reported-by Li Qiang <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/vmw/pvrdma_cmd.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Update v1: move check before page dir/tbl map
-> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02257.html

hw/rdma/vmw/pvrdma_cmd.c +5 -0

index 4f616d4177..e37fb18280 100644
@@ -259,6 +259,11 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
     int rc = -EINVAL;
     char ring_name[MAX_RING_NAME_SZ];
 
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid nchunks: %d\n", nchunks);
+        return rc;
+    }
+
     pr_dbg("pdir_dma=0x%llx\n", (long long unsigned int)pdir_dma);
     dir = rdma_pci_dma_map(pci_dev, pdir_dma, TARGET_PAGE_SIZE);
     if (!dir) {

@@ -371,6 +376,12 @@ static int create_qp_rings(PCIDevice *pci_dev, uint64_t pdir_dma,
     char ring_name[MAX_RING_NAME_SZ];
     uint32_t wqe_sz;

Signature · 1 line

2.19.2

From: Prasad J Pandit <[email protected]>

create_cq and create_qp routines allocate ring object, but it's not released in case of an error, leading to memory leakage.

Reported-by Li Qiang <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/vmw/pvrdma_cmd.c | 36 +++++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 11 deletions(-)

Update v1: define new function to free PvrdmaRing object
-> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02328.html

hw/rdma/vmw/pvrdma_cmd.c +24 -4

index e37fb18280..7e29607d2f 100644
@@ -313,6 +313,14 @@ out:
     return rc;
 }
 
+static void destroy_cq_ring(PvrdmaRing *ring)
+{
+    pvrdma_ring_free(ring);
+    /* ring_state was in slot 1, not 0 so need to jump back */
+    rdma_pci_dma_unmap(ring->dev, --ring->ring_state, TARGET_PAGE_SIZE);
+    g_free(ring);
+}
+
 static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
                      union pvrdma_cmd_resp *rsp)
 {

@@ -335,6 +343,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
 
     rc = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev, cmd->cqe,
                           &resp->cq_handle, ring);
+    if (rc) {
+        destroy_cq_ring(ring);
+    }
 
     return rc;
 }

@@ -355,10 +366,7 @@ static int destroy_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
     }
 
     ring = (PvrdmaRing *)cq->opaque;
-    pvrdma_ring_free(ring);
-    /* ring_state was in slot 1, not 0 so need to jump back */
-    rdma_pci_dma_unmap(PCI_DEVICE(dev), --ring->ring_state, TARGET_PAGE_SIZE);
-    g_free(ring);
+    destroy_cq_ring(ring);
 
     rdma_rm_dealloc_cq(&dev->rdma_dev_res, cmd->cq_handle);
 

@@ -456,6 +464,17 @@ out:
     return rc;
 }
 
+static void destroy_qp_rings(PvrdmaRing *ring)
+{
+    pr_dbg("sring=%p\n", &ring[0]);
+    pvrdma_ring_free(&ring[0]);
+    pr_dbg("rring=%p\n", &ring[1]);
+    pvrdma_ring_free(&ring[1]);
+
+    rdma_pci_dma_unmap(ring->dev, ring->ring_state, TARGET_PAGE_SIZE);
+    g_free(ring);
+}
+
 static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                      union pvrdma_cmd_resp *rsp)
 {

@@ -485,6 +504,7 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                           cmd->max_recv_sge, cmd->recv_cq_handle, rings,
                           &resp->qpn);
     if (rc) {
+        destroy_qp_rings(rings);
         return rc;
     }
 

@@ -557,13 +577,7 @@ static int destroy_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
     rdma_rm_dealloc_qp(&dev->rdma_dev_res, cmd->qp_handle);
 

hw/rdma/vmw/pvrdma_cmd.c +0 -0

     return 0;
 }

Signature · 1 line

2.19.2

From: Prasad J Pandit <[email protected]>

pvrdma_idx_ring_has_[data/space] routines also return invalid index PVRDMA_INVALID_IDX[=-1], if ring has no data/space. Check return value from these routines to avoid plausible infinite loops.

Reported-by Li Qiang <[email protected]>

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/vmw/pvrdma_dev_ring.c | 37 +++++++++++++++++------------------
 1 file changed, 18 insertions(+), 19 deletions(-)

Update v1: receive error code in idx variable, remove comments
-> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg02342.html

hw/rdma/vmw/pvrdma_dev_ring.c +4 -7

index 01247fc041..2dccac8442 100644
@@ -73,23 +73,22 @@ out:
 
 void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
 {
-    unsigned int idx = 0, offset;
+    int idx;
+    unsigned int offset, head;
 
-    /*
-    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
-           ring->ring_state->cons_head);
-    */
-
-    if (!pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx)) {
+    idx = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &head);
+    if (idx <= 0) {

hw/rdma/vmw/pvrdma_dev_ring.c +8 -12

     offset = idx * ring->elem_sz;
-    /*
-    pr_dbg("idx=%d\n", idx);
-    pr_dbg("offset=%d\n", offset);
-    */
     return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);
 }
 

@@ -105,20 +104,20 @@ void pvrdma_ring_read_inc(PvrdmaRing *ring)
 
 void *pvrdma_ring_next_elem_write(PvrdmaRing *ring)
 {
-    unsigned int idx, offset, tail;
+    int idx;
+    unsigned int offset, tail;
 
-    /*
-    pr_dbg("%s: t=%d, h=%d\n", ring->name, ring->ring_state->prod_tail,
-           ring->ring_state->cons_head);
-    */
-
-    if (!pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail)) {
+    idx = pvrdma_idx_ring_has_space(ring->ring_state, ring->max_elems, &tail);
+    if (idx <= 0) {
         pr_dbg("CQ is full\n");
         return NULL;
     }
 
     idx = pvrdma_idx(&ring->ring_state->prod_tail, ring->max_elems);
-    /* TODO: tail == idx */
+    if (idx < 0 || tail != idx) {
+        pr_dbg("invalid idx\n");
+        return NULL;
+    }
 
     offset = idx * ring->elem_sz;
     return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);

Signature · 1 line

2.19.2

From: Prasad J Pandit <[email protected]>

Replace VENDOR_ERR_NO_SGE macro with VENDOR_ERR_INV_NUM_SGE to indicate invalid number of scatter/gather elements.

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/rdma_backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/rdma/rdma_backend.c +1 -2

index bd4710d16f..c28bfbd44d 100644
@@ -37,12 +37,11 @@
 #define VENDOR_ERR_TOO_MANY_SGES    0x202
 #define VENDOR_ERR_NOMEM            0x203
 #define VENDOR_ERR_QP0              0x204
-#define VENDOR_ERR_NO_SGE           0x205
+#define VENDOR_ERR_INV_NUM_SGE      0x205
 #define VENDOR_ERR_MAD_SEND         0x206
 #define VENDOR_ERR_INVLKEY          0x207
 #define VENDOR_ERR_MR_SMALL         0x208
 #define VENDOR_ERR_INV_MAD_BUFF     0x209
-#define VENDOR_ERR_INV_NUM_SGE      0x210
 
 #define THR_NAME_LEN 16
 #define THR_POLL_TO  5000

Signature · 1 line

2.19.2

Quoted 4 lines · P J P

From: Prasad J Pandit <[email protected]>

Replace VENDOR_ERR_NO_SGE macro with VENDOR_ERR_INV_NUM_SGE to indicate invalid number of scatter/gather elements.

Suggesting a revised commit message such as:
With commit xxxx ("pvrdma: check number of pages when creating rings") VENDOR_ERR_NO_SGE is no longer in use - delete it.

(kinda ambiguity if subject says "remove" while commit message says "replace")

Quoted 27 lines

Signed-off-by Prasad J Pandit <[email protected]>

---
 hw/rdma/rdma_backend.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

hw/rdma/rdma_backend.c +1 -2

index bd4710d16f..c28bfbd44d 100644
@@ -37,12 +37,11 @@
 #define VENDOR_ERR_TOO_MANY_SGES    0x202
 #define VENDOR_ERR_NOMEM            0x203
 #define VENDOR_ERR_QP0              0x204
-#define VENDOR_ERR_NO_SGE           0x205
+#define VENDOR_ERR_INV_NUM_SGE      0x205
 #define VENDOR_ERR_MAD_SEND         0x206
 #define VENDOR_ERR_INVLKEY          0x207
 #define VENDOR_ERR_MR_SMALL         0x208
 #define VENDOR_ERR_INV_MAD_BUFF     0x209
-#define VENDOR_ERR_INV_NUM_SGE      0x210
 
 #define THR_NAME_LEN 16
 #define THR_POLL_TO  5000

Signature · 1 line

2.19.2