scsi: ufs: scsi_get_lba error fix by check cmd opcode

From: Peter Wang <[email protected]>

When ufs init without scmd->device->sector_size set, scsi_get_lba will get a wrong shift number and ubsan error. shift exponent 4294967286 is too large for 64-bit type 'sector_t' (aka 'unsigned long long')
Call scsi_get_lba only when opcode is READ_10/WRITE_10/UNMAP.

---
 drivers/scsi/ufs/ufshcd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

index 9349557b8a01..3c4caee8fb93 100644
@@ -367,7 +367,7 @@ static void ufshcd_add_uic_command_trace(struct ufs_hba *hba,
 static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 				     enum ufs_trace_str_t str_t)
 {
-	u64 lba;
+	u64 lba = 0;
 	u8 opcode = 0, group_id = 0;
 	u32 intr, doorbell;
 	struct ufshcd_lrb *lrbp = &hba->lrb[tag];

@@ -384,7 +384,6 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 		return;
 
 	opcode = cmd->cmnd[0];
-	lba = scsi_get_lba(cmd);
 
 	if (opcode == READ_10 || opcode == WRITE_10) {
 		/*

@@ -392,6 +391,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 		 */
 		transfer_len =
 		       be32_to_cpu(lrbp->ucd_req_ptr->sc.exp_data_transfer_len);
+		lba = scsi_get_lba(cmd);
 		if (opcode == WRITE_10)
 			group_id = lrbp->cmd->cmnd[6];
 	} else if (opcode == UNMAP) {

@@ -399,6 +399,7 @@ static void ufshcd_add_command_trace(struct ufs_hba *hba, unsigned int tag,
 		 * The number of Bytes to be unmapped beginning with the lba.
 		 */
 		transfer_len = blk_rq_bytes(rq);
+		lba = scsi_get_lba(cmd);
 	}
 
 	intr = ufshcd_readl(hba, REG_INTERRUPT_STATUS);

2.18.0


_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Hmm ... how can it happen that sector_size has not been set? I think that can only happen for LUNs of type SCSI DISK if sd_read_capacity() fails? If sd_read_capacity() fails I think the sd driver is expected to set the capacity to zero?

rq->__sector == -1 for flush requests and the type of that member (sector_t) is unsigned. I think that it is allowed for a shift left of an unsigned type to overflow. From the C standard: "The result of E1 << E2 is E1 left-shifted E2 bit positions; vacated bits are filled with zeros. If E1 has an unsigned type, the value of the result is E1 × 2E2 , reduced modulo one more than the maximum value representable in the result type."

Thanks,

Bart.

Hi Bart,

When scsi scan in scsi_probe_lun function, there have much INQUIRY(0x12) command with sector_size is 0.
unsigned int shift will get 4294967286 (signed -10) and an sector_t type is 64 bit. Shift 64bit right 4294967286 will have ubsan error because ubsan think shift number should be wrong and return 0 always.
BTW, we only need the lba information when read/write/unmap. Other command such as INQUIRY is useless.

static inline sector_t scsi_get_lba(struct scsi_cmnd *scmd)
{
    unsigned int shift = ilog2(scmd->device->sector_size) - SECTOR_SHIFT; <=  shift is 4294967286 (-1-9=-10)

    return blk_rq_pos(scsi_cmd_to_rq(scmd)) >> shift;  <= sector_t type >> 4294967286 will always get 0. }

Applied to 5.18/scsi-staging, thanks!

Martin K. Petersen	Oracle Linux Engineering

_______________________________________________
Linux-mediatek mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Applied to 5.18/scsi-queue, thanks!

[1/1] scsi: ufs: scsi_get_lba error fix by check cmd opcode

      https://git.kernel.org/mkp/scsi/c/2bd3b6b75946

Martin K. Petersen	Oracle Linux Engineering