| Message ID | 874naczpk5.fsf@linux.vnet.ibm.com |
|---|---|
| State | New |
| Headers | show |
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 >
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
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 >
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
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));