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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Time-of-Check to Time-of-Use (TOCTOU) / Double Fetch vulnerability in NVMe page size validation
--

--- Patch [4]: [PATCH v4 04/10] mpi3mr: Fix NVMe page size caching for non-operational devices ---
commit 84a34ae29dcd95f3e4be7fc74958e61313bd07c9
Author: Ranjan Kumar <[email protected]>
mpi3mr: Fix NVMe page size caching for non-operational devices

This commit populates the NVMe page size attribute irrespective of device
access status so firmware can process IOCTLs and report appropriate errors.
It also adds bounds checking for page_size to prevent undefined shift
behavior, and initializes timeout values via local variables to avoid race
conditions.

Closes: https://sashiko.dev/#/patchset/[email protected]?part=4
Closes: https://sashiko.dev/#/patchset/[email protected]?part=4
Closes: https://sashiko.dev/#/patchset/[email protected]?part=4

> diff --git a/drivers/scsi/mpi3mr/mpi3mr_os.c b/drivers/scsi/mpi3mr/mpi3mr_os.c
> index d2a20f2721dbf..88b1d6360dac2 100644
> --- a/drivers/scsi/mpi3mr/mpi3mr_os.c
> +++ b/drivers/scsi/mpi3mr/mpi3mr_os.c
[ ... ]
> @@ -1349,24 +1349,32 @@ static void mpi3mr_update_tgtdev(struct mpi3mr_ioc *mrioc,
>  		struct mpi3_device0_pcie_format *pcieinf =
>  		    &dev_pg0->device_specific.pcie_format;
>  		u16 dev_info = le16_to_cpu(pcieinf->device_info);
> +		u8 pgsz = MPI3MR_DEFAULT_PGSZEXP;
> +		u8 reset_to = MPI3MR_INTADMCMD_TIMEOUT;
> +		u8 abort_to = MPI3MR_INTADMCMD_TIMEOUT;
>  
>  		tgtdev->dev_spec.pcie_inf.dev_info = dev_info;
>  		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;
> +		/* Validate firmware page size to prevent undefined shift behavior */
> +		if (pcieinf->page_size >= MPI3MR_DEFAULT_PGSZEXP &&
> +		    pcieinf->page_size <= MPI3MR_MAX_PGSZEXP)
> +			pgsz = pcieinf->page_size;

[Severity: High]
Could this introduce a time-of-check to time-of-use (TOCTOU) double fetch
vulnerability?

Since pcieinf points into dev_pg0, which resides directly in the DMA-mapped
reply buffer, is it possible for a compromised or malfunctioning PCIe device
to modify pcieinf->page_size in memory between the bounds check evaluation
and the final assignment?

If the device changes the value right after the check passes, an out-of-bounds
value might be stored into pgsz, bypassing the validation and potentially
triggering the undefined shift behavior this patch aims to prevent.

Would it be safer to fetch pcieinf->page_size into a local variable using
READ_ONCE() and then perform the validation and assignment on that local copy?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4