| Message ID | 20230228171129.4094709-1-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi: megasas: Internal cdbs have 16-byte length | expand |
+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);
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
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);
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); >
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); >> > > >
> 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
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
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);
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(-)