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

[Guenter Roeck]

On Thu, Sep 01, 2022 at 08:23:52PM +0200, Paolo Bonzini wrote:
> From: John Millikin <john@john-millikin.com>
> 
> 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 <john@john-millikin.com>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1127
> Message-Id: <20220817053458.698416-1-john@john-millikin.com>
> [Fill in correct length for adapters other than ESP. - Paolo]
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  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 <linux@roeck-us.net>
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 <linux@roeck-us.net>
---
 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(&s->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, &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