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

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

[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.

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

[Philippe Mathieu-Daudé]

+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);

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

[Michael Tokarev]

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

[Fiona Ebner]

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

[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

> 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

[Fiona Ebner]

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

[Michael Tokarev]

> 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

[Guenter Roeck]

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