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 <address@hidden>
> 
> 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 <address@hidden>
> Signed-off-by: Alexey Kardashevskiy <address@hidden>
> ---
> 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
>