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

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

[Alexey Kardashevskiy]

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>
Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 hw/scsi/spapr_vscsi.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index c5ff794..cc35b1b 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -919,6 +919,34 @@ 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;
+    int rc;
+
+    vcap = &req->iu.mad.capabilities;
+    rc = spapr_vio_dma_read(&s->vdev, be64_to_cpu(vcap->buffer),
+                            &cap, be16_to_cpu(vcap->common.length));
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA write 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, be16_to_cpu(vcap->common.length));
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
+    }
+    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;
@@ -939,6 +967,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.4.rc4

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

[Alexander Graf]

On 23.08.2013, at 10:22, Alexey Kardashevskiy wrote:

> 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>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> hw/scsi/spapr_vscsi.c | 31 +++++++++++++++++++++++++++++++
> 1 file changed, 31 insertions(+)
> 
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index c5ff794..cc35b1b 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -919,6 +919,34 @@ 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;
> +    int rc;
> +
> +    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.

It's certainly better style (read: makes it easier to prove this doesn't happen when it really is important) to read the variables into local variables and reuse them there. In this case it mostly helps readability to make sure here and below are the same variables.

> +    if (rc)  {
> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");

This is a DMA read, no? Also this should be error_report.

> +    }
> +
> +    cap.flags = 0;
> +    cap.migration.ecl = 0;
> +    cap.reserve.type = 0;
> +    cap.migration.common.server_support = 0;
> +    cap.reserve.common.server_support = 0;

Is this a memset(0)?


Alex

> +    rc = spapr_vio_dma_write(&s->vdev, be64_to_cpu(vcap->buffer),
> +                             &cap, be16_to_cpu(vcap->common.length));
> +    if (rc)  {
> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
> +    }
> +    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;
> @@ -939,6 +967,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.4.rc4
> 

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

[Benjamin Herrenschmidt]

On Sun, 2013-08-25 at 17:41 +0100, Alexander Graf wrote:
> 
> 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.
> 
> It's certainly better style (read: makes it easier to prove this
> doesn't happen when it really is important) to read the variables into
> local variables and reuse them there. In this case it mostly helps
> readability to make sure here and below are the same variables.

Ugh... It's not better style at all, it's also less efficient and the
"attack" you talk about doesn't exist... All the guest can do is shoot
itself in the foot.

Ben.

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

[Paolo Bonzini]

Il 25/08/2013 22:51, Benjamin Herrenschmidt ha scritto:
> On Sun, 2013-08-25 at 17:41 +0100, Alexander Graf wrote:
>>
>> 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.
>>
>> It's certainly better style (read: makes it easier to prove this
>> doesn't happen when it really is important) to read the variables into
>> local variables and reuse them there. In this case it mostly helps
>> readability to make sure here and below are the same variables.
> 
> Ugh... It's not better style at all, it's also less efficient and the
> "attack" you talk about doesn't exist... All the guest can do is shoot
> itself in the foot.

There are certainly cases where time-of-check-to-time-of-use
vulnerability could make QEMU access uninitialized memory (or worse,
out-of-bounds arrays).  For example, you could try racing the host on
the length of a scatter/gather list.

Paolo

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

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 15:37 +0200, Paolo Bonzini wrote:
> There are certainly cases where time-of-check-to-time-of-use
> vulnerability could make QEMU access uninitialized memory (or worse,
> out-of-bounds arrays).  For example, you could try racing the host on
> the length of a scatter/gather list.

Sure, and I mentioned that too, the latest patch from Nikunj addresses
it, I still think however that it's not a good practice to copy
everything, then do the byteswaps on the result (and it defeats use of
sparse for endian checking if we ever want to do that).

Ben.

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

[Benjamin Herrenschmidt]

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).

Cheers,
Ben.

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

[Nikunj A Dadhania]

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

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

[Alexander Graf]

Am 26.08.2013 um 05:32 schrieb 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.

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;

No need for = 0.

> +    int rc = true;

Better preassign it to a value that functions you call later would set as well. Or does spapr_vio_dma_xxx return true/false?

> +
> +    vcap = &req->iu.mad.capabilities;
> +    len = be16_to_cpu(vcap->common.length);

Good.

> +    if (len > sizeof(&cap)) {

sizeof(&cap) == sizeof(long). Is this what you want here?

> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");

Please use the error framework to report errors.

> +        goto error_out;
> +    }
> +    rc = spapr_vio_dma_read(&s->vdev, be64_to_cpu(vcap->buffer),

Just move the swizzled buffer pointer into its own variable too. Makes things more consistent (easier to read).

> +                            &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;

My question stands unanswered. Is this just memset(0)?


Alex

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

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

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 06:44 +0100, Alexander Graf wrote:
> > +    cap.flags = 0;
> > +    cap.migration.ecl = 0;
> > +    cap.reserve.type = 0;
> > +    cap.migration.common.server_support = 0;
> > +    cap.reserve.common.server_support = 0;
> 
> My question stands unanswered. Is this just memset(0)?

If it is, then why bother reading it in the first place ? :-)

Note that even if it is, I'd rather keep it that way as we are
eventually going to populate some stuff and so we have just the
placeholders to do it.

Cheers,
Ben.

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

[Alexander Graf]

Am 26.08.2013 um 08:22 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:

> On Mon, 2013-08-26 at 06:44 +0100, Alexander Graf wrote:
>>> +    cap.flags = 0;
>>> +    cap.migration.ecl = 0;
>>> +    cap.reserve.type = 0;
>>> +    cap.migration.common.server_support = 0;
>>> +    cap.reserve.common.server_support = 0;
>> 
>> My question stands unanswered. Is this just memset(0)?
> 
> If it is, then why bother reading it in the first place ? :-)

Because this is a revision that incorporates other feedback from that same review, so I'm quite sure he just missed it :)

> 
> Note that even if it is, I'd rather keep it that way as we are
> eventually going to populate some stuff and so we have just the
> placeholders to do it.

Do we ever need to preserve random fields in that struct? Or would it be clearer to just set the whole thing to 0 and then go from there?

It's definitely a case-by-case decision which way is better, but setting all fields by hand gives you potential for missing to set one.


Alex

> 
> Cheers,
> Ben.
> 
> 

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> Am 26.08.2013 um 08:22 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
>
>> On Mon, 2013-08-26 at 06:44 +0100, Alexander Graf wrote:
>>>> +    cap.flags = 0;
>>>> +    cap.migration.ecl = 0;
>>>> +    cap.reserve.type = 0;
>>>> +    cap.migration.common.server_support = 0;
>>>> +    cap.reserve.common.server_support = 0;
>>> 
>>> My question stands unanswered. Is this just memset(0)?
>> 
>> If it is, then why bother reading it in the first place ? :-)
>
> Because this is a revision that incorporates other feedback from that same review, so I'm quite sure he just missed it :)

Nope, i did not miss it.

Regards
Nikunj

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

[Alexander Graf]

On 26.08.2013, at 11:08, Nikunj A Dadhania wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> Am 26.08.2013 um 08:22 schrieb Benjamin Herrenschmidt <benh@kernel.crashing.org>:
>> 
>>> On Mon, 2013-08-26 at 06:44 +0100, Alexander Graf wrote:
>>>>> +    cap.flags = 0;
>>>>> +    cap.migration.ecl = 0;
>>>>> +    cap.reserve.type = 0;
>>>>> +    cap.migration.common.server_support = 0;
>>>>> +    cap.reserve.common.server_support = 0;
>>>> 
>>>> My question stands unanswered. Is this just memset(0)?
>>> 
>>> If it is, then why bother reading it in the first place ? :-)
>> 
>> Because this is a revision that incorporates other feedback from that same review, so I'm quite sure he just missed it :)
> 
> Nope, i did not miss it.

So you ignored it on purpose?


Alex

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

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 10:43 +0200, Alexander Graf wrote:
> Do we ever need to preserve random fields in that struct? Or would it
> be clearer to just set the whole thing to 0 and then go from there?

Yeah sort of, we set/clear bits more/less based on what the guest
put in the first place, it's a bit strange.

> It's definitely a case-by-case decision which way is better, but
> setting all fields by hand gives you potential for missing to set one.

Right. I'll let Nikunj judge here as he looked at that part of the
protocol more than I have.

Cheers,
Ben.

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:
> Am 26.08.2013 um 05:32 schrieb Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>:
>
>> Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:
>> From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
>> 
>> This implements capabilities exchange between host and client.
>
> Client?

vscsi host implemented by VIOS and vscsi client running inside
the guest.

Regards
Nikunj

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:
> Am 26.08.2013 um 05:32 schrieb 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).
>> 

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

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index e9090e5..0758263 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -858,6 +858,47 @@ 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;
+    uint64_t buffer;
+    int rc;
+
+    vcap = &req->iu.mad.capabilities;
+    len = be16_to_cpu(vcap->common.length);
+    buffer = be64_to_cpu(vcap->buffer);
+    if (len > sizeof(cap)) {
+        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
+        rc = H_PARAMETER;
+        goto error_out;
+    }
+    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
+    }
+
+    /*
+     * Current implementation does not suppport any migration or
+     * reservation capabilities. Construct the response telling the
+     * guest not to use them.
+     */
+    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, 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 +919,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

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

[Alexander Graf]

On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:

> Alexander Graf <agraf@suse.de> writes:
>> Am 26.08.2013 um 05:32 schrieb 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).
>>> 
> 
> 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 | 44 ++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
> 
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index e9090e5..0758263 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -858,6 +858,47 @@ 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;
> +    uint64_t buffer;
> +    int rc;
> +
> +    vcap = &req->iu.mad.capabilities;
> +    len = be16_to_cpu(vcap->common.length);
> +    buffer = be64_to_cpu(vcap->buffer);
> +    if (len > sizeof(cap)) {
> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
> +        rc = H_PARAMETER;
> +        goto error_out;
> +    }
> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
> +    if (rc)  {
> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");

At this point cap contains random host data, no?

> +    }
> +
> +    /*
> +     * Current implementation does not suppport any migration or
> +     * reservation capabilities. Construct the response telling the
> +     * guest not to use them.
> +     */
> +    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, buffer, &cap, len);
> +    if (rc)  {
> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
> +    }
> +error_out:

This is just "out" rather than "error_out", no? We also get here when we don't hit an error.


Alex

> +    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 +919,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
> 

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:
>
>> 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>
>> ---
>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>> +    if (rc)  {
>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>
> At this point cap contains random host data, no?

Yes, and we zero it out in this case.

>
>> +    }
>> +
>> +    /*
>> +     * Current implementation does not suppport any migration or
>> +     * reservation capabilities. Construct the response telling the
>> +     * guest not to use them.
>> +     */
>> +    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, buffer, &cap, len);
>> +    if (rc)  {
>> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
>> +    }
>> +error_out:
>
> This is just "out" rather than "error_out", no? We also get here when we don't hit an error.

Yes, the label is more readable at the goto, where we set the error
return code. In case of no error return code is H_SUCCESS. So that we
send a response back to the guest.

Regards
Nikunj

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

[Alexander Graf]

On 26.08.2013, at 13:46, Nikunj A Dadhania wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:
>> 
>>> 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>
>>> ---
>>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>>> +    if (rc)  {
>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>> 
>> At this point cap contains random host data, no?
> 
> Yes, and we zero it out in this case.

Then please make this obvious to the reader. Either memset(0) it or do cap = { };. But do something that doesn't make me look up the header file to check whether you really did catch all the fields there are in the struct.

> 
>> 
>>> +    }
>>> +
>>> +    /*
>>> +     * Current implementation does not suppport any migration or
>>> +     * reservation capabilities. Construct the response telling the
>>> +     * guest not to use them.
>>> +     */
>>> +    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, buffer, &cap, len);
>>> +    if (rc)  {
>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
>>> +    }
>>> +error_out:
>> 
>> This is just "out" rather than "error_out", no? We also get here when we don't hit an error.
> 
> Yes, the label is more readable at the goto, where we set the error
> return code. In case of no error return code is H_SUCCESS. So that we
> send a response back to the guest.

... which means we're not jumping into an error-only label.


Alex
> 

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:
>>>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>>>> +    if (rc)  {
>>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>>> 
>>> At this point cap contains random host data, no?
>> 
>> Yes, and we zero it out in this case.
>
> Then please make this obvious to the reader. Either memset(0) it or do
> cap = { };. But do something that doesn't make me look up the header
> file to check whether you really did catch all the fields there are in
> the struct.

Sure

>
>> 
>>> 
>>>> +    }
>>>> +
>>>> +    /*
>>>> +     * Current implementation does not suppport any migration or
>>>> +     * reservation capabilities. Construct the response telling the
>>>> +     * guest not to use them.
>>>> +     */
>>>> +    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, buffer, &cap, len);
>>>> +    if (rc)  {
>>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
>>>> +    }
>>>> +error_out:
>>> 
>>> This is just "out" rather than "error_out", no? We also get here when we don't hit an error.
>> 
>> Yes, the label is more readable at the goto, where we set the error
>> return code. In case of no error return code is H_SUCCESS. So that we
>> send a response back to the guest.
>
> ... which means we're not jumping into an error-only label.

True.

Regards
Nikunj

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:

> On 26.08.2013, at 13:46, Nikunj A Dadhania wrote:
>
>> Alexander Graf <agraf@suse.de> writes:
>> 
>>> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:
>>> 
>>>> 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>
>>>> ---
>>>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>>>> +    if (rc)  {
>>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>>> 
>>> At this point cap contains random host data, no?
>> 
>> Yes, and we zero it out in this case.
>
> Then please make this obvious to the reader. Either memset(0) it or do
> cap = { };. But do something that doesn't make me look up the header
> file to check whether you really did catch all the fields there are in
> the struct.

Here is the patch incorporating the comments, moreover goto is not there
anymore, we are being more accomodative and copying only the size(cap)
structure even if guest is requesting for more.

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

This implements capabilities exchange between vscsi 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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
index e9090e5..030b775 100644
--- a/hw/scsi/spapr_vscsi.c
+++ b/hw/scsi/spapr_vscsi.c
@@ -858,6 +858,57 @@ 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, req_len;
+    uint64_t buffer;
+    int rc;
+
+    vcap = &req->iu.mad.capabilities;
+    req_len = len = be16_to_cpu(vcap->common.length);
+    buffer = be64_to_cpu(vcap->buffer);
+    if (len > sizeof(cap)) {
+        fprintf(stderr, "vscsi_send_capabilities: capabilities size mismatch !\n");
+
+        /*
+         * Just read and populate the structure that is known.
+         * Zero rest of the structure.
+         */
+        len = sizeof(cap);
+    }
+    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
+    }
+
+    /*
+     * Current implementation does not suppport any migration or
+     * reservation capabilities. Construct the response telling the
+     * guest not to use them.
+     */
+    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, buffer, &cap, len);
+    if (rc)  {
+        fprintf(stderr, "vscsi_send_capabilities: DMA write failure !\n");
+    }
+    if (req_len > len) {
+        /*
+         * Being paranoid and lets not worry about the error code
+         * here. Actual write of the cap is done above.
+         */
+        spapr_vio_dma_set(&s->vdev, (buffer + len), 0, (req_len - len));
+    }
+    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 +929,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

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

[Alexander Graf]

On 27.08.2013, at 07:43, Nikunj A Dadhania wrote:

> Alexander Graf <agraf@suse.de> writes:
> 
>> On 26.08.2013, at 13:46, Nikunj A Dadhania wrote:
>> 
>>> Alexander Graf <agraf@suse.de> writes:
>>> 
>>>> On 26.08.2013, at 12:58, Nikunj A Dadhania wrote:
>>>> 
>>>>> 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>
>>>>> ---
>>>>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>>>>> +    if (rc)  {
>>>>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>>>> 
>>>> At this point cap contains random host data, no?
>>> 
>>> Yes, and we zero it out in this case.
>> 
>> Then please make this obvious to the reader. Either memset(0) it or do
>> cap = { };. But do something that doesn't make me look up the header
>> file to check whether you really did catch all the fields there are in
>> the struct.
> 
> Here is the patch incorporating the comments, moreover goto is not there
> anymore, we are being more accomodative and copying only the size(cap)
> structure even if guest is requesting for more.
> 
> From: Nikunj A Dadhania <nikunj@linux.vnet.ibm.com>
> 
> This implements capabilities exchange between vscsi 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 | 54 +++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 54 insertions(+)
> 
> diff --git a/hw/scsi/spapr_vscsi.c b/hw/scsi/spapr_vscsi.c
> index e9090e5..030b775 100644
> --- a/hw/scsi/spapr_vscsi.c
> +++ b/hw/scsi/spapr_vscsi.c
> @@ -858,6 +858,57 @@ 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, req_len;
> +    uint64_t buffer;
> +    int rc;
> +
> +    vcap = &req->iu.mad.capabilities;
> +    req_len = len = be16_to_cpu(vcap->common.length);
> +    buffer = be64_to_cpu(vcap->buffer);
> +    if (len > sizeof(cap)) {
> +        fprintf(stderr, "vscsi_send_capabilities: capabilities size mismatch !\n");
> +
> +        /*
> +         * Just read and populate the structure that is known.
> +         * Zero rest of the structure.
> +         */
> +        len = sizeof(cap);
> +    }
> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
> +    if (rc)  {
> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");

... if we fail reading

> +    }
> +
> +    /*
> +     * Current implementation does not suppport any migration or
> +     * reservation capabilities. Construct the response telling the
> +     * guest not to use them.
> +     */
> +    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, buffer, &cap, len);

... but then succeed writing back the empty structure we will report "success". Is this correct? It's fine with me if it's intended, just want to make sure we're making a conscious decision about it.

The rest looks good to me. But please post new patch versions as new top level emails, not as inline replies. My scripts use patchwork and that can't really handle this case overly well (it works, but is additional work). It also makes it hard for people who just follow the thread loosely to realize that a new version is there.


Alex

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

[Nikunj A Dadhania]

Alexander Graf <agraf@suse.de> writes:
>> +static int vscsi_send_capabilities(VSCSIState *s, vscsi_req *req)
>> +{
>> +    struct viosrp_capabilities *vcap;
>> +    struct capabilities cap = { };
>> +    uint16_t len, req_len;
>> +    uint64_t buffer;
>> +    int rc;
>> +
>> +    vcap = &req->iu.mad.capabilities;
>> +    req_len = len = be16_to_cpu(vcap->common.length);
>> +    buffer = be64_to_cpu(vcap->buffer);
>> +    if (len > sizeof(cap)) {
>> +        fprintf(stderr, "vscsi_send_capabilities: capabilities size mismatch !\n");
>> +
>> +        /*
>> +         * Just read and populate the structure that is known.
>> +         * Zero rest of the structure.
>> +         */
>> +        len = sizeof(cap);
>> +    }
>> +    rc = spapr_vio_dma_read(&s->vdev, buffer, &cap, len);
>> +    if (rc)  {
>> +        fprintf(stderr, "vscsi_send_capabilities: DMA read failure !\n");
>
> ... if we fail reading
>
>> +    }
>> +
>> +    /*
>> +     * Current implementation does not suppport any migration or
>> +     * reservation capabilities. Construct the response telling the
>> +     * guest not to use them.
>> +     */
>> +    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, buffer, &cap, len);
>
> ... but then succeed writing back the empty structure we will report
> "success". Is this correct? It's fine with me if it's intended, just
> want to make sure we're making a conscious decision about it.

Yes, that should be fine if it succeeds writing it. If DMA read/write is
an issue, it should fail writing as well and we report back failure.

>
> The rest looks good to me. But please post new patch versions as new
> top level emails, not as inline replies. My scripts use patchwork and
> that can't really handle this case overly well (it works, but is
> additional work). It also makes it hard for people who just follow the
> thread loosely to realize that a new version is there.

Sure, will take care next time.

Regards
Nikunj

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

[Benjamin Herrenschmidt]

On Mon, 2013-08-26 at 10:02 +0530, Nikunj A Dadhania wrote:

> 
> 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;

The above initialization isn't useful

> +    int rc = true;
> +
> +    vcap = &req->iu.mad.capabilities;
> +    len = be16_to_cpu(vcap->common.length);
> +    if (len > sizeof(&cap)) {
                        ^ Ugh ? Why the & here ?

> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
> +        goto error_out;
> +    }

I am not 100% familiar with the protocol, could it be that we should
just read sizeof(cap) instead of erroring out or is there no way it
can be correct and have a len too long ?

> +    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));

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

[Nikunj A Dadhania]

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

> On Mon, 2013-08-26 at 10:02 +0530, Nikunj A Dadhania wrote:
>
>> 
>> 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;
>
> The above initialization isn't useful
>
>> +    int rc = true;
>> +
>> +    vcap = &req->iu.mad.capabilities;
>> +    len = be16_to_cpu(vcap->common.length);
>> +    if (len > sizeof(&cap)) {
>                         ^ Ugh ? Why the & here ?

Oops, got that wrong.

>
>> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
>> +        goto error_out;
>> +    }
>
> I am not 100% familiar with the protocol, could it be that we should
> just read sizeof(cap) instead of erroring out or is there no way it
> can be correct and have a len too long ?

If the length is incorrect, can we trust whether cap is correct or is of
the type we are expecting?

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

[Paolo Bonzini]

Il 26/08/2013 11:06, Nikunj A Dadhania ha scritto:
>>> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
>>> +        goto error_out;
>>> +    }
>>
>> I am not 100% familiar with the protocol, could it be that we should
>> just read sizeof(cap) instead of erroring out or is there no way it
>> can be correct and have a len too long ?
> 
> If the length is incorrect, can we trust whether cap is correct or is of
> the type we are expecting?

We shouldn't care, it'd be a guest bug.

If the guest is asking for say 1024 bytes, we do not have to fill all of
them.  It is in principle possible that a subsequent revision of vscsi
will make the struct larger; perhaps a bit in the first part of the
struct will tell the guest if the second part has been filled.

Unless the spec explicitly say the opposite, I would just zero the bytes
between sizeof(cap) and len.

Paolo

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

[Nikunj A Dadhania]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 26/08/2013 11:06, Nikunj A Dadhania ha scritto:
>>>> +        fprintf(stderr, "vscsi_send_capabilities: size out of bound !\n");
>>>> +        goto error_out;
>>>> +    }
>>>
>>> I am not 100% familiar with the protocol, could it be that we should
>>> just read sizeof(cap) instead of erroring out or is there no way it
>>> can be correct and have a len too long ?
>> 
>> If the length is incorrect, can we trust whether cap is correct or is of
>> the type we are expecting?
>
> We shouldn't care, it'd be a guest bug.

Then we can do a warning on the size and set only the parts supported.

This is a kind of negotiating capabilities, where the guest says that I
can support following vscsi capabilities, hypervisor if it has
implemented them should return back with affirmative for the
capabilities supported. If not, tell the guest that hypervisor cannot
support.

>
> If the guest is asking for say 1024 bytes, we do not have to fill all of
> them.  It is in principle possible that a subsequent revision of vscsi
> will make the struct larger; perhaps a bit in the first part of the
> struct will tell the guest if the second part has been filled.
>
> Unless the spec explicitly say the opposite, I would just zero the bytes
> between sizeof(cap) and len.

Makes sense. I will change the patch accordingly.

Regards
Nikunj