Am 26.08.2013 um 05:32 schrieb Nikunj A Dadhania <nik...@linux.vnet.ibm.com>:
> Benjamin Herrenschmidt <b...@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 <nik...@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 <nik...@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
>
