| Message ID | 20120124171944.GD14494@akamai.com |
|---|---|
| State | New |
| Headers | show |
On 01/24/2012 06:19 PM, Thomas Higdon wrote: > I agree that it's better to get this into a more general place. However, > I wasn't willing to pull the MIN statement up into scsi_send_command > because I don't understand the interplay between 'len' in that function > and r->iov.iov_len. I couldn't see that there was a general relationship > between these two variables that applied to both read/write commands and > other commands. That's good enough, and it should fix also the bugs with GET CONFIGURATION that Artyom reported. Two birds with a stone! Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> Kevin, shall I take this patch in the virtio-scsi series? I'll have to resubmit anyway due to the QOM changes. Paolo
Am 26.01.2012 08:50, schrieb Paolo Bonzini: > On 01/24/2012 06:19 PM, Thomas Higdon wrote: >> I agree that it's better to get this into a more general place. However, >> I wasn't willing to pull the MIN statement up into scsi_send_command >> because I don't understand the interplay between 'len' in that function >> and r->iov.iov_len. I couldn't see that there was a general relationship >> between these two variables that applied to both read/write commands and >> other commands. > > That's good enough, and it should fix also the bugs with GET > CONFIGURATION that Artyom reported. Two birds with a stone! > > Reviewed-by: Paolo Bonzini <pbonzini@redhat.com> > > Kevin, shall I take this patch in the virtio-scsi series? I'll have to > resubmit anyway due to the QOM changes. I have picked it up for the block branch. If it conflicts with your changes, you can just rebase on top of that. Kevin
On 01/26/2012 11:41 AM, Kevin Wolf wrote: >> > Kevin, shall I take this patch in the virtio-scsi series? I'll have to >> > resubmit anyway due to the QOM changes. > I have picked it up for the block branch. If it conflicts with your > changes, you can just rebase on top of that. No, it doesn't. Thanks! Paolo
On Tue, Jan 24, 2012 at 12:19:44PM -0500, Thomas Higdon wrote: > On Tue, Jan 24, 2012 at 08:53:03AM -0500, Kevin Wolf wrote: > > Am 23.01.2012 18:15, schrieb Thomas Higdon: > > > This prevents the emulated SCSI device from trying to DMA more bytes to the > > > initiator than are expected. Without this, the SCRIPTS code in the emulated LSI > > > device eventually raises a DMA interrupt for a data overrun when an INQUIRY > > > command whose buflen exceeds req->cmd.xfer is processed. It's the > > > responsibility of the client to provide a request buffer and allocation > > > length that are large enough for the result of the command. > > > > > > Signed-off-by: Thomas Higdon <thigdon@akamai.com> > > > --- > > > hw/scsi-disk.c | 3 +++ > > > 1 files changed, 3 insertions(+), 0 deletions(-) > > > > > > diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c > > > index 5d8bf53..71fe2a3 100644 > > > --- a/hw/scsi-disk.c > > > +++ b/hw/scsi-disk.c > > > @@ -477,6 +477,9 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) > > > "buffer size %zd\n", page_code, req->cmd.xfer); > > > return -1; > > > } > > > + if (buflen > req->cmd.xfer) { > > > + buflen = req->cmd.xfer; > > > + } > > > /* done with EVPD */ > > > return buflen; > > > } > > > > I wonder if it would make sense to make this check in a more central > > place like scsi_send_command(). This way we would avoid similar bugs in > > other commands. > > Limit the return value (corresponding to the length of the buffer to be > DMAed back to the intiator) to the value in req->cmd.xfer, which is the > amount of data that the initiator expects. Eliminate now-duplicate code > that does this guarding in the functions for individual commands. > > Without this, the SCRIPTS code in the emulated LSI device eventually > raises a DMA interrupt for a data overrun when an INQUIRY command whose > buflen exceeds req->cmd.xfer is processed. It's the responsibility of > the client to provide a request buffer and allocation length that are > large enough for the result of the command. > > Signed-off-by: Thomas Higdon <thigdon@akamai.com> > --- Kevin: Will you take this through your block/scsi tree? Stefan
On 01/27/2012 06:59 AM, Stefan Hajnoczi wrote: >> > Signed-off-by: Thomas Higdon<thigdon@akamai.com> >> > --- > Kevin: Will you take this through your block/scsi tree? Yes, he did already. Paolo
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index 5d8bf53..11cfe73 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -391,9 +391,6 @@ static int scsi_disk_emulate_inquiry(SCSIRequest *req, uint8_t *outbuf) } l = strlen(s->serial); - if (l > req->cmd.xfer) { - l = req->cmd.xfer; - } if (l > 20) { l = 20; } @@ -1002,9 +999,6 @@ static int scsi_disk_emulate_mode_sense(SCSIDiskReq *r, uint8_t *outbuf) outbuf[0] = ((buflen - 2) >> 8) & 0xff; outbuf[1] = (buflen - 2) & 0xff; } - if (buflen > r->req.cmd.xfer) { - buflen = r->req.cmd.xfer; - } return buflen; } @@ -1038,9 +1032,6 @@ static int scsi_disk_emulate_read_toc(SCSIRequest *req, uint8_t *outbuf) default: return -1; } - if (toclen > req->cmd.xfer) { - toclen = req->cmd.xfer; - } return toclen; } @@ -1251,6 +1242,7 @@ static int scsi_disk_emulate_command(SCSIDiskReq *r) scsi_check_condition(r, SENSE_CODE(INVALID_OPCODE)); return -1; } + buflen = MIN(buflen, req->cmd.xfer); return buflen; not_ready: