Re: [PATCH v6 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] The driver fails to check for dma_map_sg() returning 0 in mvumi_make_sgl(), leading to a zero-length scatter-gather list being submitted to the hardware.
--

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

scsi: mvumi: use 16-bits defined sense codes

This commit refactors the Marvell UMI driver to use scsi_set_sense() and
enum values from scsi_sense.h instead of hardcoded additional sense codes.

> 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);
>  	return -1;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the error
handling in this area, I noticed a pre-existing issue if DMA mapping fails
earlier in the command setup.

When processing a SCSI command with data, mvumi_build_frame() calls
mvumi_make_sgl(). Inside mvumi_make_sgl(), dma_map_sg() is called:

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) {
    ...
}

If memory is fragmented or IOMMU space is exhausted, dma_map_sg() returns 0.

Because *sg_count is 0, the check *sg_count > mhba->max_sge evaluates to
false, and the mapping loop is bypassed. The function returns 0 (success)
instead of jumping to an error path.

Back in mvumi_build_frame(), the driver proceeds to set the transfer length:

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);
    ...
}

Does this instruct the hardware to transfer data with sg_counts = 0 but a
non-zero data_transfer_length?

Could this cause a hardware DMA fault or lead to memory corruption and a
subsequent controller reset?

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