| Message ID | 20220307111752.10465-1-peter.wang@mediatek.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] scsi: ufs: scsi_get_lba error fix by check cmd opcode | expand |
On 3/7/22 03:17, peter.wang@mediatek.com wrote: > 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. 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.
}
On 3/8/22 1:52 AM, Bart Van Assche wrote:
>
> 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.
On 3/7/22 03:17, peter.wang@mediatek.com wrote: > 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. Reviewed-by: Bart Van Assche <bvanassche@acm.org>
> 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. Applied to 5.18/scsi-staging, thanks!
On Mon, 7 Mar 2022 19:17:52 +0800, peter.wang@mediatek.com wrote: > From: Peter Wang <peter.wang@mediatek.com> > > 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. > > [...] 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
diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c index 9349557b8a01..3c4caee8fb93 100644 --- a/drivers/scsi/ufs/ufshcd.c +++ b/drivers/scsi/ufs/ufshcd.c @@ -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);