| Message ID | 20101122101536.4DE10F90AD@ochil.suse.de |
|---|---|
| State | New |
| Headers | show |
On Mon, Nov 22, 2010 at 10:15 AM, Hannes Reinecke <hare@suse.de> wrote: > @@ -172,6 +170,9 @@ static void scsi_read_data(SCSIRequest *req) > /* No data transfer may already be in progress */ > assert(r->req.aiocb == NULL); > > + if (r->req.cmd.mode == SCSI_XFER_TO_DEV) > + BADF("Data transfer direction invalid\n"); > + > if (r->sector_count == (uint32_t)-1) { > DPRINTF("Read buf_len=%zd\n", r->iov[0].iov_len); > r->sector_count = 0; > @@ -284,6 +285,9 @@ static int scsi_write_data(SCSIRequest *req) > /* No data transfer may already be in progress */ > assert(r->req.aiocb == NULL); > > + if (r->req.cmd.mode != SCSI_XFER_TO_DEV) > + BADF("Data transfer direction invalid\n"); > + > n = iov_size(r->iov, r->iov_num) / 512; > if (n) { > qemu_iovec_init_external(&r->qiov, r->iov, r->iov_num); If the guest can trigger this then there must be a SCSI response (an error?). Right now BADF() will do fprintf(stderr) and then continue executing. Can we abort the operation? Stefan
On 11/23/2010 11:03 AM, Stefan Hajnoczi wrote: > On Mon, Nov 22, 2010 at 10:15 AM, Hannes Reinecke <hare@suse.de> wrote: >> @@ -172,6 +170,9 @@ static void scsi_read_data(SCSIRequest *req) >> /* No data transfer may already be in progress */ >> assert(r->req.aiocb == NULL); >> >> + if (r->req.cmd.mode == SCSI_XFER_TO_DEV) >> + BADF("Data transfer direction invalid\n"); >> + >> if (r->sector_count == (uint32_t)-1) { >> DPRINTF("Read buf_len=%zd\n", r->iov[0].iov_len); >> r->sector_count = 0; >> @@ -284,6 +285,9 @@ static int scsi_write_data(SCSIRequest *req) >> /* No data transfer may already be in progress */ >> assert(r->req.aiocb == NULL); >> >> + if (r->req.cmd.mode != SCSI_XFER_TO_DEV) >> + BADF("Data transfer direction invalid\n"); >> + >> n = iov_size(r->iov, r->iov_num) / 512; >> if (n) { >> qemu_iovec_init_external(&r->qiov, r->iov, r->iov_num); > > If the guest can trigger this then there must be a SCSI response (an > error?). Right now BADF() will do fprintf(stderr) and then continue > executing. > > Can we abort the operation? > I've done a patch for it as per suggestion by hch. Right now we have if (r->req.cmd.mode == SCSI_XFER_TO_DEV) { DPRINTF("Data transfer direction invalid\n"); scsi_read_complete(r, -EINVAL); return; } and -EINVAL will return the sense code 'INVALID FIELD IN CDB'. Will be in the next patchset. Cheers, Hannes
On Tue, Nov 23, 2010 at 10:12 AM, Hannes Reinecke <hare@suse.de> wrote: > On 11/23/2010 11:03 AM, Stefan Hajnoczi wrote: >> On Mon, Nov 22, 2010 at 10:15 AM, Hannes Reinecke <hare@suse.de> wrote: >>> @@ -172,6 +170,9 @@ static void scsi_read_data(SCSIRequest *req) >>> /* No data transfer may already be in progress */ >>> assert(r->req.aiocb == NULL); >>> >>> + if (r->req.cmd.mode == SCSI_XFER_TO_DEV) >>> + BADF("Data transfer direction invalid\n"); >>> + >>> if (r->sector_count == (uint32_t)-1) { >>> DPRINTF("Read buf_len=%zd\n", r->iov[0].iov_len); >>> r->sector_count = 0; >>> @@ -284,6 +285,9 @@ static int scsi_write_data(SCSIRequest *req) >>> /* No data transfer may already be in progress */ >>> assert(r->req.aiocb == NULL); >>> >>> + if (r->req.cmd.mode != SCSI_XFER_TO_DEV) >>> + BADF("Data transfer direction invalid\n"); >>> + >>> n = iov_size(r->iov, r->iov_num) / 512; >>> if (n) { >>> qemu_iovec_init_external(&r->qiov, r->iov, r->iov_num); >> >> If the guest can trigger this then there must be a SCSI response (an >> error?). Right now BADF() will do fprintf(stderr) and then continue >> executing. >> >> Can we abort the operation? >> > I've done a patch for it as per suggestion by hch. > Right now we have > > if (r->req.cmd.mode == SCSI_XFER_TO_DEV) { > DPRINTF("Data transfer direction invalid\n"); > scsi_read_complete(r, -EINVAL); > return; > } > > and -EINVAL will return the sense code 'INVALID FIELD IN CDB'. > Will be in the next patchset. Sounds good. Stefan
diff --git a/hw/scsi-disk.c b/hw/scsi-disk.c index a0dda88..82072a8 100644 --- a/hw/scsi-disk.c +++ b/hw/scsi-disk.c @@ -52,8 +52,6 @@ typedef struct SCSIDiskState SCSIDiskState; typedef struct SCSIDiskReq { SCSIRequest req; - /* ??? We should probably keep track of whether the data transfer is - a read or a write. Currently we rely on the host getting it right. */ /* Both sector and sector_count are in terms of qemu 512 byte blocks. */ uint64_t sector; uint32_t sector_count; @@ -172,6 +170,9 @@ static void scsi_read_data(SCSIRequest *req) /* No data transfer may already be in progress */ assert(r->req.aiocb == NULL); + if (r->req.cmd.mode == SCSI_XFER_TO_DEV) + BADF("Data transfer direction invalid\n"); + if (r->sector_count == (uint32_t)-1) { DPRINTF("Read buf_len=%zd\n", r->iov[0].iov_len); r->sector_count = 0; @@ -284,6 +285,9 @@ static int scsi_write_data(SCSIRequest *req) /* No data transfer may already be in progress */ assert(r->req.aiocb == NULL); + if (r->req.cmd.mode != SCSI_XFER_TO_DEV) + BADF("Data transfer direction invalid\n"); + n = iov_size(r->iov, r->iov_num) / 512; if (n) { qemu_iovec_init_external(&r->qiov, r->iov, r->iov_num); @@ -970,11 +974,9 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) SCSIDiskReq *r = DO_UPCAST(SCSIDiskReq, req, req); SCSIDiskState *s = DO_UPCAST(SCSIDiskState, qdev, req->dev); ssize_t len = 0; - int is_write; uint8_t command; command = buf[0]; - is_write = 0; DPRINTF("Command: lun=%d tag=0x%x data=0x%02x", lun, tag, buf[0]); if (scsi_req_parse(&r->req, buf) != 0) { @@ -1057,7 +1059,6 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) goto illegal_lba; } r->sector = r->req.cmd.lba * s->cluster_size; - is_write = 1; break; case MODE_SELECT: DPRINTF("Mode Select(6) (len %lu)\n", (long)r->req.cmd.xfer); @@ -1098,7 +1099,7 @@ static int32_t scsi_send_command(SCSIRequest *req, uint8_t *buf) scsi_command_complete(r, GOOD, SENSE_CODE(NO_SENSE)); } len += r->sector_count * 512; - if (is_write) { + if (r->req.cmd.mode == SCSI_XFER_TO_DEV) { return -len; } else { if (!r->sector_count)
scsi_req_parse() already provides for a data direction setting, so we should be using it to check for correct direction. Signed-off-by: Hannes Reinecke <hare@suse.de> --- hw/scsi-disk.c | 13 +++++++------ 1 files changed, 7 insertions(+), 6 deletions(-)