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