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

[Alexander Graf]

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

> Alexander Graf <address@hidden> writes:
>> Am 26.08.2013 um 05:32 schrieb Nikunj A Dadhania <address@hidden>:
>> 
>>> Benjamin Herrenschmidt <address@hidden> 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 <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>
> ---
> 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
>