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