Re: [PATCH v2 22/40] scsi: myrX: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Data Corruption: Incorrect offset used for writing the SCSI mode parameter block descriptor corrupts the 24-bit block length.
- [High] Command Timeout: The REQUEST_SENSE command in myrb_ldev_queuecommand returns without calling scsi_done(), leading to a hanging command.
- [High] Resource Leak: Returning SCSI_MLQUEUE_HOST_BUSY after a successful scsi_dma_map() without unmapping causes a DMA mapping leak.
- [High] Memory Leak: DMA pool allocations are leaked on the SG mapping error path in myrs_queuecommand.
--

commit 3eaecf993e28ec2a9d1c5ee8b1fc650d068a56f8
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: myrX: use 16-bits defined sense codes

Refactor the Mylex RAID controller driver to use scsi_set_sense() and the
16-bits sense_code field of struct scsi_sense_hdr and replace all
hard-coded additional sense codes and additional sense code qualifiers
with the enum values defined in include/scsi/scsi_sense.h. This helps with
code clarity as the sense codes being processed are easier to test and
self-documented.

> diff --git a/drivers/scsi/myrb.c b/drivers/scsi/myrb.c
> index 3678b66310ed0..e7d796add9864 100644
> --- a/drivers/scsi/myrb.c
> +++ b/drivers/scsi/myrb.c
[ ... ]
> @@ -1398,7 +1400,7 @@ myrb_mode_sense(struct myrb_hba *cb, struct scsi_cmnd *scmd,
>  static void myrb_request_sense(struct myrb_hba *cb,
>  		struct scsi_cmnd *scmd)
>  {
> -	scsi_build_sense(scmd, 0, NO_SENSE, 0, 0);
> +	scsi_set_sense(scmd, 0, NO_SENSE, NO_ADDITIONAL_SENSE_INFORMATION);
>  	scsi_sg_copy_from_buffer(scmd, scmd->sense_buffer,
>  				 SCSI_SENSE_BUFFERSIZE);
>  }

[Severity: High]
This is a pre-existing issue, but looking at how myrb_request_sense() is used
in myrb_ldev_queuecommand() in drivers/scsi/myrb.c:

	case REQUEST_SENSE:
		myrb_request_sense(cb, scmd);
		scmd->result = (DID_OK << 16);
		return 0;

Because returning 0 indicates to the SCSI midlayer that the driver has
accepted the command and will complete it asynchronously, does failing to call
scsi_done(scmd) here cause the command to hang indefinitely and eventually
trigger a SCSI timeout?

[ ... ]
> @@ -1461,8 +1463,8 @@ static enum scsi_qc_status myrb_ldev_queuecommand(struct Scsi_Host *shost,
>  	case MODE_SENSE:
>  		if ((scmd->cmnd[2] & 0x3F) != 0x3F &&
>  		    (scmd->cmnd[2] & 0x3F) != 0x08) {
> -			/* Illegal request, invalid field in CDB */
> -			scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x24, 0);
> +			scsi_set_sense(scmd, 0, ILLEGAL_REQUEST,
> +				       INVALID_FIELD_IN_CDB);
>  		} else {
>  			myrb_mode_sense(cb, scmd, ldev_info);

[Severity: Critical]
This is a pre-existing issue, but I noticed myrb_mode_sense() in
drivers/scsi/myrb.c writes the block size to the mode page block descriptor
using put_unaligned_be32():

static void myrb_mode_sense(struct myrb_hba *cb, struct scsi_cmnd *scmd,
		struct myrb_ldev_info *ldev_info)
{
...
		put_unaligned_be32(cb->ldev_block_size, &block_desc[5]);

Since the block length in the descriptor is a 24-bit field located at offsets
5, 6, and 7, won't writing a 32-bit value at offset 5 overflow the LSB into
offset 8 (the start of the mode page header)? For a typical 512 byte block
size, it seems this writes 2 instead of 512, completely corrupting capacity
reporting.

>  			scmd->result = (DID_OK << 16);
[ ... ]
> @@ -1544,8 +1546,8 @@ static enum scsi_qc_status myrb_ldev_queuecommand(struct Scsi_Host *shost,
>  		block_cnt = get_unaligned_be32(&scmd->cmnd[6]);
>  		break;
>  	default:
> -		/* Illegal request, invalid opcode */
> -		scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x20, 0);
> +		scsi_set_sense(scmd, 0, ILLEGAL_REQUEST,
> +			       INVALID_COMMAND_OP_CODE);
>  		scsi_done(scmd);
>  		return 0;
>  	}

[Severity: High]
This is a pre-existing issue, but later in myrb_ldev_queuecommand() in
drivers/scsi/myrb.c, after the switch statement, the function maps DMA memory:

	nsge = scsi_dma_map(scmd);
    ...
	} else {
		struct myrb_sge *hw_sgl;
		dma_addr_t hw_sgl_addr;
		int i;

		hw_sgl = dma_pool_alloc(cb->sg_pool, GFP_ATOMIC, &hw_sgl_addr);
		if (!hw_sgl)
			return SCSI_MLQUEUE_HOST_BUSY;

Does returning SCSI_MLQUEUE_HOST_BUSY without calling scsi_dma_unmap() cause
a DMA mapping and IOMMU resource leak when the midlayer retries the command
and maps it again?

> diff --git a/drivers/scsi/myrs.c b/drivers/scsi/myrs.c
> index afd68225221a6..1192b01fab8ec 100644
> --- a/drivers/scsi/myrs.c
> +++ b/drivers/scsi/myrs.c
[ ... ]
> @@ -1603,7 +1607,8 @@ static enum scsi_qc_status myrs_queuecommand(struct Scsi_Host *shost,
>  
>  	switch (scmd->cmnd[0]) {
>  	case REPORT_LUNS:
> -		scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x20, 0x0);
> +		scsi_set_sense(scmd, 0, ILLEGAL_REQUEST,
> +			       INVALID_COMMAND_OP_CODE);
>  		scsi_done(scmd);
>  		return 0;
>  	case MODE_SENSE:
> @@ -1612,8 +1617,8 @@ static enum scsi_qc_status myrs_queuecommand(struct Scsi_Host *shost,
>  
>  			if ((scmd->cmnd[2] & 0x3F) != 0x3F &&
>  			    (scmd->cmnd[2] & 0x3F) != 0x08) {
> -				/* Illegal request, invalid field in CDB */
> -				scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x24, 0);
> +				scsi_set_sense(scmd, 0, ILLEGAL_REQUEST,
> +					       INVALID_FIELD_IN_CDB);
>  			} else {
>  				myrs_mode_sense(cs, scmd, ldev_info);

[Severity: Critical]
This is a pre-existing issue, but myrs_mode_sense() in drivers/scsi/myrs.c
has the identical bug as myrb_mode_sense() when writing the mode page block
descriptor:

static void myrs_mode_sense(struct myrs_hba *cs, struct scsi_cmnd *scmd,
		struct myrs_ldev_info *ldev_info)
{
...
		put_unaligned_be32(ldev_info->devsize_bytes, &block_desc[5]);

Does this similarly corrupt the 24-bit block length by writing a 32-bit value?

[Severity: High]
Additionally, this is another pre-existing issue in myrs_queuecommand() in
drivers/scsi/myrs.c. When processing commands with scatterlists, if
dma_pool_alloc() fails it returns SCSI_MLQUEUE_HOST_BUSY:

			hw_sgl = dma_pool_alloc(cs->sg_pool, GFP_ATOMIC,
						&hw_sgl_addr);
			if (WARN_ON(!hw_sgl)) {
				if (cmd_blk->dcdb) {
					dma_pool_free(cs->dcdb_pool,
						      cmd_blk->dcdb,
						      cmd_blk->dcdb_dma);
...
				return SCSI_MLQUEUE_HOST_BUSY;
			}

Since scsi_dma_map() was already called earlier in the function, does
returning here without calling scsi_dma_unmap() leak the DMA mapping?

[Severity: High]
Finally, in the same scatterlist loop in myrs_queuecommand(), there is a
sanity check on hw_sgl:

		scsi_for_each_sg(scmd, sgl, nsge, i) {
			if (WARN_ON(!hw_sgl)) {
				scsi_dma_unmap(scmd);
				scmd->result = (DID_ERROR << 16);
				scsi_done(scmd);
				return 0;
			}

When taking this error path, it calls scsi_done(scmd) but bypasses the normal
completion routine myrs_handle_scsi(). Does this permanently leak the
dma_pool_alloc() blocks for cmd_blk->sense and cmd_blk->dcdb?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903034201.112211-1-dlemoal@kernel.org?part=22