Re: [PATCH v1 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unvalidated firmware-provided `page_size` used as a shift exponent can lead to Undefined Behavior and a divide-by-zero panic.
--

commit f5cd5ed48fb67b9c92c834f74f43b66abd174677
Author: Ranjan Kumar <ranjan.kumar@broadcom.com>

mpi3mr: Fix NVMe page size caching for non-operational devices

This commit updates the driver to populate the PCIe page size attribute
regardless of the device access status, ensuring management IOCTLs can be
processed for non-operational devices.

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
> @@ -1354,12 +1354,10 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
>  		tgtdev->dev_spec.pcie_inf.capb =
>  		    le32_to_cpu(pcieinf->capabilities);
>  		tgtdev->dev_spec.pcie_inf.mdts = MPI3MR_DEFAULT_MDTS;
> -		/* 2^12 = 4096 */
> -		tgtdev->dev_spec.pcie_inf.pgsz = 12;
> +		tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;

[Severity: High]
This is a pre-existing issue, but does removing the NO_ERRORS check expose
the driver to undefined behavior if the firmware returns garbage data?

For devices in an error state, the firmware might return uninitialized
data like 0xFF for pcieinf->page_size. This unvalidated value is later
used in shift operations:

drivers/scsi/mpi3mr/mpi3mr_app.c:mpi3mr_build_nvme_prp() {
    ...
    dev_pgsz = 1 << (tgtdev->dev_spec.pcie_inf.pgsz);
    ...
}

and

drivers/scsi/mpi3mr/mpi3mr_os.c:mpi3mr_configure_nvme_dev() {
    ...
    lim->virt_boundary_mask = (1 << pgsz) - 1;
    ...
}

Since 1 is a 32-bit signed integer, evaluating 1 << pgsz with a value
of 31 or greater results in signed integer overflow or a shift out of
bounds. If the shift evaluates to 0 on some architectures, it could also
cause a divide-by-zero panic when computing MPI3MR_IOCTL_SGE_SIZE % dev_pgsz
later in the code.

Is it possible to add bounds checking for the page_size before assigning
it to tgtdev->dev_spec.pcie_inf.pgsz?

>  		if (dev_pg0->access_status == MPI3_DEVICE0_ASTATUS_NO_ERRORS) {
>  			tgtdev->dev_spec.pcie_inf.mdts =
>  			    le32_to_cpu(pcieinf->maximum_data_transfer_size);
> -			tgtdev->dev_spec.pcie_inf.pgsz = pcieinf->page_size;
>  			tgtdev->dev_spec.pcie_inf.reset_to =
>  			    max_t(u8, pcieinf->controller_reset_to,
>  			     MPI3MR_INTADMCMD_TIMEOUT);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626114109.43685-1-ranjan.kumar@broadcom.com?part=4