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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

hw/rdma/rdma_backend.c +0 -0

index d7a4bbd91f..0b3b98a94c 100644

hw/rdma/rdma_backend.c +0 -0

         return;
     }

Signature · 1 line

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 +5 -0

index ca5fa8d981..a6211d416d 100644
@@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = {
     },
 };
 
+static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0;
+}
+
 static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
     PVRDMADev *dev = opaque;

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

     .write = uar_write,
     .endianness = DEVICE_LITTLE_ENDIAN,
     .impl = {

Signature · 1 line

2.19.2

Quoted 29 lines · P J P

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 +5 -0

index ca5fa8d981..a6211d416d 100644
@@ -455,6 +455,11 @@ static const MemoryRegionOps regs_ops = {
     },
 };
 
+static uint64_t uar_read(void *opaque, hwaddr addr, unsigned size)
+{
+    return 0;
+}
+
 static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 {
     PVRDMADev *dev = opaque;

@@ -496,6 +501,7 @@ static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
 }
 

hw/rdma/vmw/pvrdma_main.c static void uar_write(void *opaque, hwaddr addr, uint64_t val, unsigned size) · line 504

Quoted 2 lines

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

 static const MemoryRegionOps uar_ops = {
+    .read = uar_read,

Are you sure it is needed?

Looking at memory_region_dispatch_read1 i can see that there is a check but >not sure this is the right place. Anyways, if it is not, i believe this should be framework responsibility.

Quoted 1 line

Quoted 1 line

.write = uar_write, >> .endianness = DEVICE_LITTLE_ENDIAN, >> .impl = { >> -- >> 2.19.2 >>

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

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

index 4faeb21631..ee2888259c 100644
@@ -273,6 +273,10 @@ static int create_cq_ring(PCIDevice *pci_dev , PvrdmaRing **ring,
         pr_dbg("Failed to map to CQ page table\n");
         goto out;
     }
+    if (!nchunks || nchunks > PVRDMA_MAX_FAST_REG_PAGES) {
+        pr_dbg("invalid nchunks: %d\n", nchunks);
+        goto out;
+    }
 
     r = g_malloc(sizeof(*r));
     *ring = r;

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

     sr = g_malloc(2 * sizeof(*rr));
     rr = &sr[1];

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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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

index ee2888259c..e8d99f29fa 100644

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

+        goto out;
+    }
 
     resp->max_send_wr = cmd->max_send_wr;
     resp->max_recv_wr = cmd->max_recv_wr;

Signature · 1 line

2.19.2

Quoted 16 lines · P J P

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 | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

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

index ee2888259c..e8d99f29fa 100644

hw/rdma/vmw/pvrdma_cmd.c static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req, · line 341

Quoted 6 lines

hw/rdma/vmw/pvrdma_cmd.c +2 -1

@@ -337,7 +337,9 @@ static int create_cq(PVRDMADev *dev, union pvrdma_cmd_req *req,
 
     resp->hdr.err = rdma_rm_alloc_cq(&dev->rdma_dev_res, &dev->backend_dev,
                                      cmd->cqe, &resp->cq_handle, ring);
-    resp->cqe = cmd->cqe;
+    if (resp->hdr.err) {
+        g_free(ring);

This is not enough since all ring's resources (ring state and ring's pages) left mapped.

The steps needed are the steps detailed in destroy_cq.

hw/rdma/vmw/pvrdma_cmd.c static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req, · line 496

Quoted 10 lines

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

+    }
 
 out:
     pr_dbg("ret=%d\n", resp->hdr.err);

@@ -490,6 +492,10 @@ static int create_qp(PVRDMADev *dev, union pvrdma_cmd_req *req,
                                      cmd->max_send_sge, cmd->send_cq_handle,
                                      cmd->max_recv_wr, cmd->max_recv_sge,
                                      cmd->recv_cq_handle, rings, &resp->qpn);
+    if (resp->hdr.err) {
+        g_free(rings);

Ditto, here send rind and recv rings stays mapped. Look at how QP's ring is destroyed in destroy_qp.

For both case suggesting to define a new static function that destroy rings and call it from both error flow of create_* and from destroy_*

Quoted 5 lines

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

+        goto out;
+    }
 
     resp->max_send_wr = cmd->max_send_wr;
     resp->max_recv_wr = cmd->max_recv_wr;

Quoted 3 lines

Signature · 1 line

2.19.2

+-- On Wed, 12 Dec 2018, P J P wrote --+
| | Also, can you rebase this patch on top of the patchset i posted last week:
| | https://patchwork.kernel.org/patch/10705439/
| 
| Okay, I'll send revised patch set. Thanks so much for the prompt review.

I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But it does not seem to apply cleanly.

===

$ git branch rdma v3.1.0-rc0
$ git checkout rdma 
Switched to branch 'rdma'
$ git apply --check /tmp/add-support-for-RDMA-MAD.patch
error: patch failed: Makefile.objs:1
error: Makefile.objs: patch does not apply
error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
===

I skipped one of the makefile.objs/.json patch, applied others and then hand-fixed one of the patch pvrdma_cmd.c.

After that when I try to build QEMU, it fails with

===

 $ ./configure --prefix=/tmp/ --enable-debug
      --target-list='x86_64-softmmu x86_64-linux-user'
      --enable-rdma --with-sdlabi=2.0
 $ make
../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function 
‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration]  qapi_event_send_rdma_gid_status_changed(ifname, true,
...
../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No 
such file or directory
 #include "qapi/qapi-events-rdma.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
===

Not sure if other patches are required. Any idea?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

+-- On Wed, 12 Dec 2018, P J P wrote --+
| | Also, can you rebase this patch on top of the patchset i posted last week:
| | https://patchwork.kernel.org/patch/10705439/
| 
| Okay, I'll send revised patch set. Thanks so much for the prompt review.

I tried to git apply above patch-set over v3.1.0-rc2, v3.1.0-rc0, v3.0.0. But it does not seem to apply cleanly.

===

$ git branch rdma v3.1.0-rc0

$ git checkout rdma 
Switched to branch 'rdma'
$ git apply --check /tmp/add-support-for-RDMA-MAD.patch
error: patch failed: Makefile.objs:1
error: Makefile.objs: patch does not apply
error: patch failed: hw/rdma/vmw/pvrdma_cmd.c:224
error: hw/rdma/vmw/pvrdma_cmd.c: patch does not apply
===

I skipped one of the makefile.objs/.json patch, applied others and then hand-fixed one of the patch pvrdma_cmd.c.

After that when I try to build QEMU, it fails with

===

 $ ./configure --prefix=/tmp/ --enable-debug
      --target-list='x86_64-softmmu x86_64-linux-user'
      --enable-rdma --with-sdlabi=2.0
 $ make
../qemu/hw/rdma/rdma_backend.c:1132:5: error: implicit declaration of function 
‘qapi_event_send_rdma_gid_status_changed’ [-Werror=implicit-function-declaration]  qapi_event_send_rdma_gid_status_changed(ifname, true,
...
../qemu/hw/rdma/rdma_backend.c:22:10: fatal error: qapi/qapi-events-rdma.h: No 
such file or directory
 #include "qapi/qapi-events-rdma.h"
          ^~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
===

Not sure if other patches are required. Any idea?

Thank you.
--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

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 | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

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

index 01247fc041..61b6a0a869 100644
@@ -73,6 +73,7 @@ out:
 
 void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
 {
+    int e;
     unsigned int idx = 0, offset;
 
     /*

@@ -80,7 +81,8 @@ void *pvrdma_ring_next_elem_read(PvrdmaRing *ring)
            ring->ring_state->cons_head);
     */
 
-    if (!pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx)) {
+    e = pvrdma_idx_ring_has_data(ring->ring_state, ring->max_elems, &idx);
+    if (e <= 0) {
         pr_dbg("No more data in ring\n");
         return NULL;
     }

@@ -105,20 +107,24 @@ 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) {

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

     offset = idx * ring->elem_sz;
     return ring->pages[offset / TARGET_PAGE_SIZE] + (offset % TARGET_PAGE_SIZE);

Signature · 1 line

2.19.2