linux-kernel - Re: [PATCH] scsi: ips: fix missing break in switch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 18 Oct 2018 13:37:24 +1100 (AEDT)
From:   Finn Thain <fthain@...egraphics.com.au>
To:     "Martin K. Petersen" <martin.petersen@...cle.com>
cc:     "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        Adaptec OEM Raid Solutions <aacraid@...rosemi.com>,
        "James E.J. Bottomley" <jejb@...ux.vnet.ibm.com>,
        linux-scsi@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: ips: fix missing break in switch

On Wed, 17 Oct 2018, Martin K. Petersen wrote:

> 
> >> See the case statements above for another fast exit scenario.
> >> 
> >
> > But that's an error path.
> 
> Look further down. Several other SCSI commands are completed as NOPs the 
> same way.
> 

That's true, but it doesn't indicate a bug to me.

On the contrary, the entire switch (scb->scsi_cmd->cmnd[0]) statement in 
ips_send_cmd() appears to be carefully constructed, just like the matching 
statement in ips_chkstatus().

But none of these indications can decide the question. We just don't have 
enough information. (I'm sure that someone somewhere does have the 
relevant technical information...)

If this really is undecidable, then I think the right patch is the more 
prudent one. That is, add a "fall through" comment, not a "break" 
statement.

Or perhaps a "fall through (TODO: check this)" comment.

> Also, I don't see how the case statement for TUR/INQUIRY would do 
> anything meaningful in terms of preparing a START STOP UNIT for the 
> firmware.
> 

If SSU case doesn't do anything meaningful, then neither does the 
TUR/INQUIRY case, and then you can just delete all of that code:

                        } else {
                                scb->cmd.logical_info.op_code = IPS_CMD_GET_LD_INFO;
                                scb->cmd.logical_info.command_id = IPS_COMMAND_ID(ha, scb);
                                scb->cmd.logical_info.reserved = 0;
                                scb->cmd.logical_info.reserved2 = 0;
                                scb->data_len = sizeof (IPS_LD_INFO);
                                scb->data_busaddr = ha->logical_drive_info_dma_addr;
                                scb->flags = 0;
                                scb->cmd.logical_info.buffer_addr = scb->data_busaddr;
                                ret = IPS_SUCCESS;
                        }

FWIW, I think this line of reasoning is mistaken.

--