1. Patchew
  2. QEMU
  3. View series

[PATCH] scsi: megasas: Internal cdbs have 16-byte length

Guenter Roeck posted 1 patch 2 years, 11 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230228171129.4094709-1-linux@roeck-us.net
Maintainers: Hannes Reinecke <hare@suse.com>, Paolo Bonzini <pbonzini@redhat.com>, Fam Zheng <fam@euphon.net>
hw/scsi/megasas.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
[PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Guenter Roeck 2 years, 11 months ago 
Host drivers do not necessarily set cdb_len in megasas io commands.
With commits 6d1511cea0 ("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 Linux from affected
SCSI drives because cdb_len is set to 0 by the host driver.
Set the cdb length to its actual size to solve the problem.

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.39.1
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Paolo Bonzini 1 year, 2 months ago 
Queued, thanks.

Paolo
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Fiona Ebner 2 years, 11 months ago 
Am 28.02.23 um 18:11 schrieb Guenter Roeck:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("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 Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.
> 

Tested-by: Fiona Ebner <f.ebner@proxmox.com>

But I do have a question:

> 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;
> -    }

Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
consequence of

> Host drivers do not necessarily set cdb_len in megasas io commands.

that this can be uninitialized memory and we need to assume it was not
explicitly set?

Best Regards,
Fiona

> -
>      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);
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Guenter Roeck 2 years, 11 months ago 
On 3/3/23 01:02, Fiona Ebner wrote:
> Am 28.02.23 um 18:11 schrieb Guenter Roeck:
>> Host drivers do not necessarily set cdb_len in megasas io commands.
>> With commits 6d1511cea0 ("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 Linux from affected
>> SCSI drives because cdb_len is set to 0 by the host driver.
>> Set the cdb length to its actual size to solve the problem.
>>
> 
> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
> 
> But I do have a question:
> 
>> 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;
>> -    }
> 
> Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
> consequence of
> 
>> Host drivers do not necessarily set cdb_len in megasas io commands.
> 
> that this can be uninitialized memory and we need to assume it was not
> explicitly set?
> 

I doubt that real hardware uses or checks the field for the affected commands
because that would be pointless, but it is really up to you to decide how
you want to handle it.

Guenter

> Best Regards,
> Fiona
> 
>> -
>>       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);
>
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Fiona Ebner 2 years, 11 months ago 
Am 03.03.23 um 16:10 schrieb Guenter Roeck:
> On 3/3/23 01:02, Fiona Ebner wrote:
>> Am 28.02.23 um 18:11 schrieb Guenter Roeck:
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>> With commits 6d1511cea0 ("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 Linux from affected
>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>> Set the cdb length to its actual size to solve the problem.
>>>
>>
>> Tested-by: Fiona Ebner <f.ebner@proxmox.com>
>>
>> But I do have a question:
>>
>>> 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;
>>> -    }
>>
>> Shouldn't we still fail when cmd->frame->header.cdb_len > 16? Or is the
>> consequence of
>>
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>
>> that this can be uninitialized memory and we need to assume it was not
>> explicitly set?
>>
> 
> I doubt that real hardware uses or checks the field for the affected
> commands
> because that would be pointless, but it is really up to you to decide how
> you want to handle it.
> 
> Guenter

Okay, thank you for the explanation!

> 
>> Best Regards,
>> Fiona
>>
>>> -
>>>       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);
>>
> 
> 
>
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Michael Tokarev 2 years, 11 months ago 
28.02.2023 20:11, Guenter Roeck wrote:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("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 Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.

This smells like a -stable material (CC qemu-stable@).

/mjt
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Michael Tokarev 1 year, 11 months ago 
> 28.02.2023 20:11, Guenter Roeck wrote:
>> Host drivers do not necessarily set cdb_len in megasas io commands.
>> With commits 6d1511cea0 ("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 Linux from affected
>> SCSI drives because cdb_len is set to 0 by the host driver.
>> Set the cdb length to its actual size to solve the problem.

Has this been lost/forgotten?

/mjt
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Guenter Roeck 1 year, 11 months ago 
On 2/17/24 01:06, Michael Tokarev wrote:
>> 28.02.2023 20:11, Guenter Roeck wrote:
>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>> With commits 6d1511cea0 ("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 Linux from affected
>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>> Set the cdb length to its actual size to solve the problem.
> 
> Has this been lost/forgotten?
> 

Not sure. My understanding was that I could not prove that this is how
real hardware handles the situation, thus it wasn't applied. I carry it
locally in my builds of qemu, so it is not a problem for me. Note that
I didn't check if the problem has since been fixed differently. Maybe
that is the case and the problem no longer exists in the upstream version
of qemu.

Guenter
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Fabiano Rosas 1 year, 2 months ago 
Guenter Roeck <linux@roeck-us.net> writes:

> On 2/17/24 01:06, Michael Tokarev wrote:
>>> 28.02.2023 20:11, Guenter Roeck wrote:
>>>> Host drivers do not necessarily set cdb_len in megasas io commands.
>>>> With commits 6d1511cea0 ("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 Linux from affected
>>>> SCSI drives because cdb_len is set to 0 by the host driver.
>>>> Set the cdb length to its actual size to solve the problem.
>> 
>> Has this been lost/forgotten?
>> 
>
> Not sure. My understanding was that I could not prove that this is how
> real hardware handles the situation, thus it wasn't applied. I carry it
> locally in my builds of qemu, so it is not a problem for me. Note that
> I didn't check if the problem has since been fixed differently. Maybe
> that is the case and the problem no longer exists in the upstream version
> of qemu.

The issue still happens upstream and I have just written the exact same
fix for it, so I'm adding my:

Reviewed-by: Fabiano Rosas <farosas@suse.de>
Re: [PATCH] scsi: megasas: Internal cdbs have 16-byte length
Posted by Philippe Mathieu-Daudé 2 years, 11 months ago 
+Hannes

On 28/2/23 18:11, Guenter Roeck wrote:
> Host drivers do not necessarily set cdb_len in megasas io commands.
> With commits 6d1511cea0 ("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 Linux from affected
> SCSI drives because cdb_len is set to 0 by the host driver.
> Set the cdb length to its actual size to solve the problem.
> 
> 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);

I haven't checked the spec, but this logic makes sense to me, so:

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

>       if (!cmd->req) {
>           trace_megasas_scsi_req_alloc_failed(
>               mfi_frame_desc(frame_cmd), target_id, lun_id);

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.