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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Failure of dma_map_sg() is incorrectly treated as success, leading to potential data corruption.
--

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

scsi: mvumi: use 16-bits defined sense codes
    
Refactor the Marvell UMI driver to use scsi_set_sense() 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.
    
No functional change intended.

> diff --git a/drivers/scsi/mvumi.c b/drivers/scsi/mvumi.c
> index e70d336b4ab39..a5bd3258a3994 100644
> --- a/drivers/scsi/mvumi.c
> +++ b/drivers/scsi/mvumi.c
> @@ -2067,7 +2067,7 @@ static unsigned char mvumi_build_frame(struct mvumi_hba *mhba,
>  	return 0;
>  
>  error:
> -	scsi_build_sense(scmd, 0, ILLEGAL_REQUEST, 0x24, 0);
> +	scsi_set_sense(scmd, 0, ILLEGAL_REQUEST, INVALID_FIELD_IN_CDB);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how we reach this
error block, does the driver mishandle a dma_map_sg() failure?

In mvumi_build_frame(), we call mvumi_make_sgl(). If dma_map_sg() fails
inside mvumi_make_sgl(), it returns 0:

drivers/scsi/mvumi.c:mvumi_make_sgl() {
    ...
	*sg_count = dma_map_sg(&mhba->pdev->dev, scsi_sglist(scmd), sgnum,
			       scmd->sc_data_direction);
	if (*sg_count > mhba->max_sge) {
        ...
		return -1;
	}
    ...
    return 0;
}

Because 0 is not greater than mhba->max_sge, the error check is skipped,
and mvumi_make_sgl() returns 0 (success) instead of -1. 

Back in mvumi_build_frame(), this bypasses the goto error:

drivers/scsi/mvumi.c:mvumi_build_frame() {
    ...
	if (scsi_bufflen(scmd)) {
		if (mvumi_make_sgl(mhba, scmd, &pframe->payload[0],
			&pframe->sg_counts))
			goto error;

		pframe->data_transfer_length = scsi_bufflen(scmd);
	}
    ...
}

Will this cause the hardware to reuse stale DMA addresses from a previous
command frame since data_transfer_length is set but sg_counts is 0?

>  	return -1;
>  }
>

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