Re: [Qemu-devel] [PATCH] spapr-vscsi: Adding VSCSI capabilities

[Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> On Sun, 2013-08-25 at 17:41 +0100, Alexander Graf wrote:
>> > +    vcap = &req->iu.mad.capabilities;
>> > +    rc = spapr_vio_dma_read(&s->vdev, be64_to_cpu(vcap->buffer),
>> > +                            &cap,
>> be16_to_cpu(vcap->common.length));
>> 
>> While I don't think any harm could happen from it, this could lead to
>> a potential timing attack where we read and write from different
>> locations in memory if the guest swizzles the request while we're
>> processing it.
>
> BTW. While I disagree with your initial comment ... is there any bound
> checking here ? That looks like potential stack corruption unless I
> miss something if the guest passes a too big length...
>
> So at least the length should be read once, bound-checked, then the read
> done with the result (don't bound check and read again, that would be
> indeed racy).

How about the below patch:


From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>

This implements capabilities exchange between host and client.
As at the moment no capability is supported, put zero flags everywhere
and return.

Signed-off-by: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
---
 hw/scsi/spapr_vscsi.c | 37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index e9090e5..fae3644 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -858,6 +858,40 @@ static int vscsi_send_adapter_info(VSCSIState *s, vscsi_req *req)
     return vscsi_send_iu(s, req, sizeof(*sinfo), VIOSRP_MAD_FORMAT);
 }
 
+static int vscsi_send_capabilities(VSCSIState *s, vscsi_req *req)
+{
+    struct viosrp_capabilities *vcap;
+    struct capabilities cap;
+    uint16_t len = 0;
+    int rc = true;
+
+    vcap = &req->iu.mad.capabilities;
+    len = be16_to_cpu(vcap->common.length);
+    if (len > sizeof(&cap)) {
+        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
+        goto error_out;
+    }
+    rc = spapr_vio_dma_read(&s->vdev, be64_to_cpu(vcap->buffer),
+                            &cap, len);
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
+    }
+
+    cap.flags = 0;
+    cap.migration.ecl = 0;
+    cap.reserve.type = 0;
+    cap.migration.common.server_support = 0;
+    cap.reserve.common.server_support = 0;
+    rc = spapr_vio_dma_write(&s->vdev, be64_to_cpu(vcap->buffer),
+                             &cap, len);
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
+    }
+error_out:
+    vcap->common.status = rc ? cpu_to_be32(1) : 0;
+    return vscsi_send_iu(s, req, sizeof(*vcap), VIOSRP_MAD_FORMAT);
+}
+
 static int vscsi_handle_mad_req(VSCSIState *s, vscsi_req *req)
 {
     union mad_iu *mad = &req->iu.mad;
@@ -878,6 +912,9 @@ static int vscsi_handle_mad_req(VSCSIState *s, vscsi_req *req)
         mad->host_config.common.status = cpu_to_be16(1);
         vscsi_send_iu(s, req, sizeof(mad->host_config), VIOSRP_MAD_FORMAT);
         break;
+    case VIOSRP_CAPABILITIES_TYPE:
+        vscsi_send_capabilities(s, req);
+        break;
     default:
         fprintf(stderr, "VSCSI: Unknown MAD type %02x\n",
                 be32_to_cpu(mad->empty_iu.common.type));
-- 
1.8.3.1