Re: [PATCH] mtd: rawnand: gpmi: Fix possible NULL pointer dereference in gpmi_nfc_exec_op()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] mtd: rawnand: gpmi: lost error and OOB access in gpmi_nfc_exec_op()
--

commit 17db1082d69f752c7e093324fef48f49184391f5
Author: Han Xu <han.xu@nxp.com>

mtd: rawnand: gpmi: Fix possible NULL pointer dereference in gpmi_nfc_exec_op()

This commit moves the NULL check on the DMA descriptor pointer in
gpmi_nfc_exec_op() from inside the instruction loop to after the loop
completes.

> diff --git a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> index 527165ccc839d..333ba468ecaf6 100644
> --- a/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c
> @@ -2616,11 +2616,11 @@ static int gpmi_nfc_exec_op(struct nand_chip *chip,
>  						   &direct);
>  			break;
>  		}
> +	}
>  
> -		if (!desc) {
> -			ret = -ENXIO;
> -			goto unmap;
> -		}
> +	if (!desc) {
> +		ret = -ENXIO;
> +		goto unmap;
>  	}

[Severity: Critical]
Does moving the !desc check outside the instruction loop mask failures on
intermediate instructions and allow out-of-bounds accesses on
this->transfers[]?

In gpmi_nfc_exec_op(), each iteration of the for (i = 0; i < op->ninstrs; i++)
loop overwrites desc with the descriptor returned by gpmi_chain_wait_ready(),
gpmi_chain_command(), gpmi_chain_data_write(), or gpmi_chain_data_read().

If an intermediate instruction fails to prepare a DMA descriptor and sets desc
to NULL, the loop no longer aborts. When mxs_dma_prep_slave_sg() in
drivers/dma/mxs-dma.c fails on an intermediate instruction, it sets
mxs_chan->status = DMA_ERROR and returns NULL:

drivers/dma/mxs-dma.c:mxs_dma_prep_slave_sg() {
	...
	if (mxs_chan->status == DMA_IN_PROGRESS)
		idx = mxs_chan->desc_count;

	if (sg_len + idx > NUM_CCW) {
		dev_err(mxs_dma->dma_device.dev,
				"maximum number of sg exceeded: %d > %d\n",
				sg_len, NUM_CCW);
		goto err_out;
	}

	mxs_chan->status = DMA_IN_PROGRESS;
	...
err_out:
	mxs_chan->status = DMA_ERROR;
	return NULL;
}

On the next instruction in the loop, mxs_dma_prep_slave_sg() sees
mxs_chan->status != DMA_IN_PROGRESS, resets idx to 0, overwrites
mxs_chan->ccw[0], and returns a non-NULL descriptor. The post-loop !desc check
then passes and submits a truncated DMA chain to hardware.

In addition, get_next_transfer() checks for array exhaustion using exact
equality after incrementing this->ntransfers:

drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c:get_next_transfer() {
	struct gpmi_transfer *transfer = &this->transfers[this->ntransfers];

	this->ntransfers++;

	if (this->ntransfers == GPMI_MAX_TRANSFERS)
		return NULL;

	return transfer;
}

Previously, returning NULL immediately aborted the loop via goto unmap. If the
loop continues after this->ntransfers reaches GPMI_MAX_TRANSFERS, a subsequent
call to get_next_transfer() indexes past the end of this->transfers[],
increments this->ntransfers past GPMI_MAX_TRANSFERS, and returns the
out-of-bounds pointer, which also causes the unmap cleanup loop to iterate
out of bounds:

	for (i = 0; i < this->ntransfers; i++) {
		struct gpmi_transfer *transfer = &this->transfers[i];

		if (transfer->direction != DMA_NONE)
			dma_unmap_sg(this->dev, &transfer->sgl, 1,
				     transfer->direction);
	}

Also, the continue in the NAND_OP_CMD_INSTR case is guarded by:

	if (i + 1 != op->ninstrs &&
	    op->instrs[i + 1].type == NAND_OP_ADDR_INSTR)
		continue;

so the next iteration is guaranteed to execute the NAND_OP_ADDR_INSTR case and
check desc. Should the !desc check inside the loop be kept (or present in both
places to also handle a zero-length NAND_OP_DATA_IN_INSTR)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105103410.4424-1-stefan.ghetu@nxp.com?part=1

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/