| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <68c54d56-3e44-4f43-8bd6-f6b7fa1f379b@linaro.org>
Date: Fri, 30 May 2025 09:56:13 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Gabor Juhos <j4g8y7@...il.com>, Mark Brown <broonie@...nel.org>,
Md Sadre Alam <quic_mdalam@...cinc.com>,
Varadarajan Narayanan <quic_varada@...cinc.com>,
Sricharan Ramabadhran <quic_srichara@...cinc.com>,
Miquel Raynal <miquel.raynal@...tlin.com>,
Richard Weinberger <richard@....at>, Vignesh Raghavendra <vigneshr@...com>
Cc: linux-spi@...r.kernel.org, linux-mtd@...ts.infradead.org,
linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
Lakshmi Sowjanya D <quic_laksd@...cinc.com>
Subject: Re: [PATCH v2 2/2] mtd: nand: qpic_common: prevent out of bounds
access of BAM arrays
On 29/05/2025 18:25, Gabor Juhos wrote:
> The common QPIC code does not do any boundary checking when it handles
> the command elements and scatter gater list arrays of a BAM transaction,
> thus it allows to access out of bounds elements in those.
>
> Although it is the responsibility of the given driver to allocate enough
> space for all possible BAM transaction variations, however there can be
> mistakes in the driver code which can lead to hidden memory corruption
> issues which are hard to debug.
>
> This kind of problem has been observed during testing the 'spi-qpic-snand'
> driver. Although the driver has been fixed with a preceding patch, but it
> still makes sense to reduce the chance of having such errors again later.
>
> In order to prevent such errors, change the qcom_alloc_bam_transaction()
> function to store the number of elements of the arrays in the
> 'bam_transaction' strucutre during allocation. Also, add sanity checks to
> the qcom_prep_bam_dma_desc_{cmd,data}() functions to avoid using out of
> bounds indices for the arrays.
>
> Tested-by: Lakshmi Sowjanya D <quic_laksd@...cinc.com> # on SDX75
> Signed-off-by: Gabor Juhos <j4g8y7@...il.com>
> ---
> Changes in v2:
> - remove the inline qcom_err_bam_array_full() function and print the error
> messages directly from the respective functions instead
> - add 'Tested-by' tag from Lakshmi Sowjanya D, and remove the
> "Tested with the 'spi-qpic-snand' driver only." line from the
> commit message as SDX75 uses the qcom_nandc driver
> - move the note about of the preferred merging order into the cover letter
> ---
> drivers/mtd/nand/qpic_common.c | 30 ++++++++++++++++++++++++++----
> include/linux/mtd/nand-qpic-common.h | 8 ++++++++
> 2 files changed, 34 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c
> index e0ed25b5afea9b289b767cd3d9c2d7572ed52008..30f17d959300cc7448d0c2e9e2516c52655494f0 100644
> --- a/drivers/mtd/nand/qpic_common.c
> +++ b/drivers/mtd/nand/qpic_common.c
> @@ -57,14 +57,15 @@ qcom_alloc_bam_transaction(struct qcom_nand_controller *nandc)
> bam_txn_buf += sizeof(*bam_txn);
>
> bam_txn->bam_ce = bam_txn_buf;
> - bam_txn_buf +=
> - sizeof(*bam_txn->bam_ce) * QPIC_PER_CW_CMD_ELEMENTS * num_cw;
> + bam_txn->bam_ce_nitems = QPIC_PER_CW_CMD_ELEMENTS * num_cw;
> + bam_txn_buf += sizeof(*bam_txn->bam_ce) * bam_txn->bam_ce_nitems;
>
> bam_txn->cmd_sgl = bam_txn_buf;
> - bam_txn_buf +=
> - sizeof(*bam_txn->cmd_sgl) * QPIC_PER_CW_CMD_SGL * num_cw;
> + bam_txn->cmd_sgl_nitems = QPIC_PER_CW_CMD_SGL * num_cw;
> + bam_txn_buf += sizeof(*bam_txn->cmd_sgl) * bam_txn->cmd_sgl_nitems;
>
> bam_txn->data_sgl = bam_txn_buf;
> + bam_txn->data_sgl_nitems = QPIC_PER_CW_DATA_SGL * num_cw;
>
> init_completion(&bam_txn->txn_done);
>
> @@ -237,6 +238,11 @@ int qcom_prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read,
> struct bam_cmd_element *bam_ce_buffer;
> struct bam_transaction *bam_txn = nandc->bam_txn;
>
> + if (bam_txn->bam_ce_pos + size > bam_txn->bam_ce_nitems) {
> + dev_err(nandc->dev, "BAM %s array is full\n", "CE");
> + return -EINVAL;
> + }
> +
> bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_pos];
>
> /* fill the command desc */
> @@ -258,6 +264,12 @@ int qcom_prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read,
>
> /* use the separate sgl after this command */
> if (flags & NAND_BAM_NEXT_SGL) {
> + if (bam_txn->cmd_sgl_pos >= bam_txn->cmd_sgl_nitems) {
> + dev_err(nandc->dev, "BAM %s array is full\n",
> + "CMD sgl");
> + return -EINVAL;
> + }
> +
> bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_start];
> bam_ce_size = (bam_txn->bam_ce_pos -
> bam_txn->bam_ce_start) *
> @@ -297,10 +309,20 @@ int qcom_prep_bam_dma_desc_data(struct qcom_nand_controller *nandc, bool read,
> struct bam_transaction *bam_txn = nandc->bam_txn;
>
> if (read) {
> + if (bam_txn->rx_sgl_pos >= bam_txn->data_sgl_nitems) {
> + dev_err(nandc->dev, "BAM %s array is full\n", "RX sgl");
> + return -EINVAL;
> + }
> +
> sg_set_buf(&bam_txn->data_sgl[bam_txn->rx_sgl_pos],
> vaddr, size);
> bam_txn->rx_sgl_pos++;
> } else {
> + if (bam_txn->tx_sgl_pos >= bam_txn->data_sgl_nitems) {
> + dev_err(nandc->dev, "BAM %s array is full\n", "TX sgl");
> + return -EINVAL;
> + }
> +
> sg_set_buf(&bam_txn->data_sgl[bam_txn->tx_sgl_pos],
> vaddr, size);
> bam_txn->tx_sgl_pos++;
> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
> index cd7172e6c1bbffeee0363a14044980a72ea17723..3ca4073a496b8fd2a99112a9caefd3f110260568 100644
> --- a/include/linux/mtd/nand-qpic-common.h
> +++ b/include/linux/mtd/nand-qpic-common.h
> @@ -240,6 +240,9 @@
> * @last_data_desc - last DMA desc in data channel (tx/rx).
> * @last_cmd_desc - last DMA desc in command channel.
> * @txn_done - completion for NAND transfer.
> + * @bam_ce_nitems - the number of elements in the @bam_ce array
> + * @cmd_sgl_nitems - the number of elements in the @cmd_sgl array
> + * @data_sgl_nitems - the number of elements in the @data_sgl array
> * @bam_ce_pos - the index in bam_ce which is available for next sgl
> * @bam_ce_start - the index in bam_ce which marks the start position ce
> * for current sgl. It will be used for size calculation
> @@ -258,6 +261,11 @@ struct bam_transaction {
> struct dma_async_tx_descriptor *last_data_desc;
> struct dma_async_tx_descriptor *last_cmd_desc;
> struct completion txn_done;
> +
> + unsigned int bam_ce_nitems;
> + unsigned int cmd_sgl_nitems;
> + unsigned int data_sgl_nitems;
> +
> struct_group(bam_positions,
> u32 bam_ce_pos;
> u32 bam_ce_start;
>
> --
> 2.49.0
>
>
This one doesn't apply to -next
deckard
{~/Development/worktree/reviews/linux-next-25-05-30-daily-reviews}±(linux-next-25-05-30-daily-reviews);
greetings, earthling [1.052Mb]$ ☞ b4 shazam
20250529-qpic-snand-avoid-mem-corruption-v2-0-2f0d13afc7d2@gm
Grabbing thread from
lore.kernel.org/all/20250529-qpic-snand-avoid-mem-corruption-v2-0-2f0d13afc7d2@...il.com/t.mbox.gz
Checking for newer revisions
Grabbing search results from lore.kernel.org
Analyzing 3 messages in the thread
Analyzing 12 code-review messages
Checking attestation on all messages, may take a moment...
---
✓ [PATCH v2 1/2] spi: spi-qpic-snand: reallocate BAM transactions
✓ [PATCH v2 2/2] mtd: nand: qpic_common: prevent out of bounds access
of BAM arrays
---
✓ Signed: DKIM/gmail.com
---
Total patches: 2
---
Base: using specified base-commit b00d6864a4c948529dc6ddd2df76bf175bf27c63
Applying: spi: spi-qpic-snand: reallocate BAM transactions
Applying: mtd: nand: qpic_common: prevent out of bounds access of BAM arrays
Patch failed at 0002 mtd: nand: qpic_common: prevent out of bounds
access of BAM arrays
error: patch failed: drivers/mtd/nand/qpic_common.c:237
error: drivers/mtd/nand/qpic_common.c: patch does not apply
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am
--abort".
hint: Disable this message with "git config set advice.mergeConflict false"
deckard
{~/Development/worktree/reviews/linux-next-25-05-30-daily-reviews}±(linux-next-25-05-30-daily-reviews);
greetings, earthling [1.052Mb]$ ☞ git-log-graph
* 4ae57ce867d8f - (HEAD -> linux-next-25-05-30-daily-reviews) spi:
spi-qpic-snand: reallocate BAM transactions (8 seconds ago)
Powered by blists - more mailing lists