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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing check for scsi_dma_map() returning an error (-ENOMEM), leading to out-of-bounds DMA, uninitialized DMA memory usage, or memory corruption.
- [High] Missing scsi_done() call on REQUEST_SENSE command completion.
--

commit 3566a2a2b43f8a90937be5314f52e6262a307f13
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
[ ... ]
> @@ -1493,8 +1495,8 @@ static enum scsi_qc_status myrb_ldev_queuecommand(struct Scsi_Host *shost,
>  		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does this code miss a call to
scsi_done() on REQUEST_SENSE command completion?

A SCSI command completes but its completion callback is never invoked. The block
layer and SCSI midlayer are left waiting indefinitely for the command to finish,
leading to an I/O hang followed by SCSI timeout and error recovery:

drivers/scsi/myrb.c:myrb_ldev_queuecommand() {
    ...
	case REQUEST_SENSE:
		myrb_request_sense(cb, scmd);
		scmd->result = (DID_OK << 16);
		return 0;
    ...
}

>  	case SEND_DIAGNOSTIC:
>  		if (scmd->cmnd[1] != 0x04) {
> -			/* 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 {
[ ... ]
> @@ -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 does this code proceed without checking if
scsi_dma_map() returned a negative error code (like -ENOMEM)?

If scsi_dma_map() fails, the driver does not check for < 0 and assigns the
negative value to mbox->type5.sg_count (which is cast to an unsigned 8-bit
integer, e.g. -12 becomes 244):

drivers/scsi/myrb.c:myrb_ldev_queuecommand() {
    ...
	nsge = scsi_dma_map(scmd);
	if (nsge == 1) {
    ...
	} else {
    ...
		mbox->type5.sg_count = nsge;
    ...
}

This would cause the hardware to execute a scatter-gather DMA reading hundreds
of uninitialized SGEs from memory, leading to wild out-of-bounds DMA
reads/writes, memory corruption, or IOMMU faults.

A similar issue exists in drivers/scsi/myrs.c where the result is only checked
against 1:

drivers/scsi/myrs.c:myrs_queuecommand() {
    ...
	nsge = scsi_dma_map(scmd);
	if (nsge == 1) {
    ...
	} else {
    ...
		if (nsge > 2) {
    ...
}

Are these missing failure checks for scsi_dma_map() intentional?

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