Re: [PULL 02/39] scsi: Add buf_len parameter to scsi_req_new()

2022-12-07 Thread Guenter Roeck 
On Thu, Sep 01, 2022 at 08:23:52PM +0200, Paolo Bonzini wrote:
> From: John Millikin 
> 
> When a SCSI command is received from the guest, the CDB length implied
> by the first byte might exceed the number of bytes the guest sent. In
> this case scsi_req_new() will read uninitialized data, causing
> unpredictable behavior.
> 
> Adds the buf_len parameter to scsi_req_new() and plumbs it through the
> call stack.
> 
> Signed-off-by: John Millikin 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1127
> Message-Id: <20220817053458.698416-1-j...@john-millikin.com>
> [Fill in correct length for adapters other than ESP. - Paolo]
> Signed-off-by: Paolo Bonzini 
> ---
>  hw/scsi/esp.c  |  2 +-
>  hw/scsi/lsi53c895a.c   |  2 +-
>  hw/scsi/megasas.c  | 10 +-

...

> @@ -1823,7 +1823,7 @@ static int megasas_handle_io(MegasasState *s, 
> MegasasCmd *cmd, int frame_cmd)
>  
>  megasas_encode_lba(cdb, lba_start, lba_count, is_write);
>  cmd->req = scsi_req_new(sdev, cmd->index,
> -lun_id, cdb, cmd);
> +lun_id, cdb, cdb_len, cmd);

This doesn't work for me. cdb is a local array in this function,
its contents are filled in the function, and Linux doesn't set the
cdb_len field for io requests (or, rather, treats it as reserved
field and sets it to 0). This results in Linux boot failures when
trying to boot from the affected controllers.

The patch below fixes the problem for me, though I have no idea if
it is correct.

Guenter

---
>From 687093dc7e48ce42de22c5675a1005890f014f22 Mon Sep 17 00:00:00 2001
From: Guenter Roeck 
Date: Wed, 7 Dec 2022 13:45:07 -0800
Subject: [PATCH] scsi: megasas: Internal cdbs have 16-byte length

Linux does not set cdb_len in megasas io commands. With commits d1511cea0
("scsi: Reject commands if the CDB length exceeds buf_len") and fe9d8927e2
("scsi: Add buf_len parameter to scsi_req_new()"), this results in
failures to boot from affected SCSI drives because cdb_len is 0.

Signed-off-by: Guenter Roeck 
---
 hw/scsi/megasas.c | 14 ++
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index 9cbbb16121..d624866bb6 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1780,7 +1780,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd 
*cmd, int frame_cmd)
 uint8_t cdb[16];
 int len;
 struct SCSIDevice *sdev = NULL;
-int target_id, lun_id, cdb_len;
+int target_id, lun_id;
 
 lba_count = le32_to_cpu(cmd->frame->io.header.data_len);
 lba_start_lo = le32_to_cpu(cmd->frame->io.lba_lo);
@@ -1789,7 +1789,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd 
*cmd, int frame_cmd)
 
 target_id = cmd->frame->header.target_id;
 lun_id = cmd->frame->header.lun_id;
-cdb_len = cmd->frame->header.cdb_len;
 
 if (target_id < MFI_MAX_LD && lun_id == 0) {
 sdev = scsi_device_find(>bus, 0, target_id, lun_id);
@@ -1804,15 +1803,6 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd 
*cmd, int frame_cmd)
 return MFI_STAT_DEVICE_NOT_FOUND;
 }
 
-if (cdb_len > 16) {
-trace_megasas_scsi_invalid_cdb_len(
-mfi_frame_desc(frame_cmd), 1, target_id, lun_id, cdb_len);
-megasas_write_sense(cmd, SENSE_CODE(INVALID_OPCODE));
-cmd->frame->header.scsi_status = CHECK_CONDITION;
-s->event_count++;
-return MFI_STAT_SCSI_DONE_WITH_ERROR;
-}
-
 cmd->iov_size = lba_count * sdev->blocksize;
 if (megasas_map_sgl(s, cmd, >frame->io.sgl)) {
 megasas_write_sense(cmd, SENSE_CODE(TARGET_FAILURE));
@@ -1823,7 +1813,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd 
*cmd, int frame_cmd)
 
 megasas_encode_lba(cdb, lba_start, lba_count, is_write);
 cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cdb_len, cmd);
+lun_id, cdb, sizeof(cdb), cmd);
 if (!cmd->req) {
 trace_megasas_scsi_req_alloc_failed(
 mfi_frame_desc(frame_cmd), target_id, lun_id);
-- 
2.36.2

[PULL 02/39] scsi: Add buf_len parameter to scsi_req_new()

2022-09-01 Thread Paolo Bonzini 
From: John Millikin 

When a SCSI command is received from the guest, the CDB length implied
by the first byte might exceed the number of bytes the guest sent. In
this case scsi_req_new() will read uninitialized data, causing
unpredictable behavior.

Adds the buf_len parameter to scsi_req_new() and plumbs it through the
call stack.

Signed-off-by: John Millikin 
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1127
Message-Id: <20220817053458.698416-1-j...@john-millikin.com>
[Fill in correct length for adapters other than ESP. - Paolo]
Signed-off-by: Paolo Bonzini 
---
 hw/scsi/esp.c  |  2 +-
 hw/scsi/lsi53c895a.c   |  2 +-
 hw/scsi/megasas.c  | 10 +-
 hw/scsi/mptsas.c   |  3 ++-
 hw/scsi/scsi-bus.c | 21 +
 hw/scsi/scsi-disk.c|  7 ---
 hw/scsi/scsi-generic.c |  5 +++--
 hw/scsi/spapr_vscsi.c  |  3 ++-
 hw/scsi/virtio-scsi.c  |  5 +++--
 hw/scsi/vmw_pvscsi.c   |  2 +-
 hw/usb/dev-storage.c   |  2 +-
 hw/usb/dev-uas.c   |  5 +++--
 include/hw/scsi/scsi.h | 11 ++-
 13 files changed, 45 insertions(+), 33 deletions(-)

diff --git a/hw/scsi/esp.c b/hw/scsi/esp.c
index c799c19bd4..2ff18ce500 100644
--- a/hw/scsi/esp.c
+++ b/hw/scsi/esp.c
@@ -292,7 +292,7 @@ static void do_command_phase(ESPState *s)
 esp_fifo_pop_buf(>cmdfifo, buf, cmdlen);
 
 current_lun = scsi_device_find(>bus, 0, s->current_dev->id, s->lun);
-s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, s);
+s->current_req = scsi_req_new(current_lun, 0, s->lun, buf, cmdlen, s);
 datalen = scsi_req_enqueue(s->current_req);
 s->ti_size = datalen;
 fifo8_reset(>cmdfifo);
diff --git a/hw/scsi/lsi53c895a.c b/hw/scsi/lsi53c895a.c
index ad5f5e5f39..05a43ec807 100644
--- a/hw/scsi/lsi53c895a.c
+++ b/hw/scsi/lsi53c895a.c
@@ -864,7 +864,7 @@ static void lsi_do_command(LSIState *s)
 s->current = g_new0(lsi_request, 1);
 s->current->tag = s->select_tag;
 s->current->req = scsi_req_new(dev, s->current->tag, s->current_lun, buf,
-   s->current);
+   s->dbc, s->current);
 
 n = scsi_req_enqueue(s->current->req);
 if (n) {
diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
index d5dfb412ba..7082456d65 100644
--- a/hw/scsi/megasas.c
+++ b/hw/scsi/megasas.c
@@ -1062,7 +1062,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, 
int lun,
 info->inquiry_data[0] = 0x7f; /* Force PQual 0x3, PType 0x1f */
 info->vpd_page83[0] = 0x7f;
 megasas_setup_inquiry(cmdbuf, 0, sizeof(info->inquiry_data));
-cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, sizeof(cmdbuf), 
cmd);
 if (!cmd->req) {
 trace_megasas_dcmd_req_alloc_failed(cmd->index,
 "PD get info std inquiry");
@@ -1080,7 +1080,7 @@ static int megasas_pd_get_info_submit(SCSIDevice *sdev, 
int lun,
 return MFI_STAT_INVALID_STATUS;
 } else if (info->inquiry_data[0] != 0x7f && info->vpd_page83[0] == 0x7f) {
 megasas_setup_inquiry(cmdbuf, 0x83, sizeof(info->vpd_page83));
-cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, cmd);
+cmd->req = scsi_req_new(sdev, cmd->index, lun, cmdbuf, sizeof(cmdbuf), 
cmd);
 if (!cmd->req) {
 trace_megasas_dcmd_req_alloc_failed(cmd->index,
 "PD get info vpd inquiry");
@@ -1268,7 +1268,7 @@ static int megasas_ld_get_info_submit(SCSIDevice *sdev, 
int lun,
 cmd->iov_buf = g_malloc0(dcmd_size);
 info = cmd->iov_buf;
 megasas_setup_inquiry(cdb, 0x83, sizeof(info->vpd_page83));
-cmd->req = scsi_req_new(sdev, cmd->index, lun, cdb, cmd);
+cmd->req = scsi_req_new(sdev, cmd->index, lun, cdb, sizeof(cdb), cmd);
 if (!cmd->req) {
 trace_megasas_dcmd_req_alloc_failed(cmd->index,
 "LD get info vpd inquiry");
@@ -1748,7 +1748,7 @@ static int megasas_handle_scsi(MegasasState *s, 
MegasasCmd *cmd,
 return MFI_STAT_SCSI_DONE_WITH_ERROR;
 }
 
-cmd->req = scsi_req_new(sdev, cmd->index, lun_id, cdb, cmd);
+cmd->req = scsi_req_new(sdev, cmd->index, lun_id, cdb, cdb_len, cmd);
 if (!cmd->req) {
 trace_megasas_scsi_req_alloc_failed(
 mfi_frame_desc(frame_cmd), target_id, lun_id);
@@ -1823,7 +1823,7 @@ static int megasas_handle_io(MegasasState *s, MegasasCmd 
*cmd, int frame_cmd)
 
 megasas_encode_lba(cdb, lba_start, lba_count, is_write);
 cmd->req = scsi_req_new(sdev, cmd->index,
-lun_id, cdb, cmd);
+lun_id, cdb, cdb_len, cmd);
 if (!cmd->req) {
 trace_megasas_scsi_req_alloc_failed(
 mfi_frame_desc(frame_cmd), target_id, lun_id);
diff --git a/hw/scsi/mptsas.c b/hw/scsi/mptsas.c
index 706cf0df3a..a90c2546f1