Message ID | 20250525-qpic-snand-avoid-mem-corruption-v1-2-5fe528def7fb@gmail.com |
---|---|
State | New |
Headers | show |
Series | spi: spi-qpic-snand: avoid memory corruption | expand |
Hi, On 5/25/2025 10:35 PM, 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 with the 'spi-qpic-snand' driver only. I recommend testing this patch on both the IPQ and SDX platforms, as the QPIC raw NAND driver are utilized across both. If you have access to IPQ and SDX devices with raw NAND, please proceed with testing on both. Otherwise, I can handle testing on the IPQ raw NAND device and coordinate with Lakshmi Sowjanya D (quic_laksd@quicinc.com) for testing on the SDX platform. Thanks, Alam.
Hi Gabor, On 25/05/2025 at 19:05:36 +02, Gabor Juhos <j4g8y7@gmail.com> 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 with the 'spi-qpic-snand' driver only. > > Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> > --- > Preferably, this should go in via the SPI tree along with the previous > patch. It is not a strict requirement though, in the case it gets > included separately through the mtd tree it reveals the bug fixed in > the first patch. Sorry, didn't see that in the first place. Fine by me. > --- > drivers/mtd/nand/qpic_common.c | 28 ++++++++++++++++++++++++---- > include/linux/mtd/nand-qpic-common.h | 8 ++++++++ > 2 files changed, 32 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c > index e0ed25b5afea9b289b767cd3d9c2d7572ed52008..fb1f81e4bdacaa3e81660a20e164926c64633513 100644 > --- a/drivers/mtd/nand/qpic_common.c > +++ b/drivers/mtd/nand/qpic_common.c > @@ -15,6 +15,13 @@ > #include <linux/slab.h> > #include <linux/mtd/nand-qpic-common.h> > > +static inline int qcom_err_bam_array_full(struct qcom_nand_controller *nandc, > + const char *name) > +{ > + dev_err(nandc->dev, "BAM %s array is full\n", name); > + return -EINVAL; > +} This is rather uncommon, I don't know if it's very relevant to do that. Please drop this static inline function. Thanks, Miquèl
2025. 05. 26. 8:53 keltezéssel, Md Sadre Alam írta: > Hi, > > On 5/25/2025 10:35 PM, 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 with the 'spi-qpic-snand' driver only. > I recommend testing this patch on both the IPQ and SDX platforms, > as the QPIC raw NAND driver are utilized across both. > > If you have access to IPQ and SDX devices with raw NAND, please proceed > with testing on both. Sorry, I have no SDX devices at all, and unfortunately I can't access my older IPQ boards before next week. > > Otherwise, I can handle testing on the IPQ raw NAND device and coordinate with > Lakshmi Sowjanya D (quic_laksd@quicinc.com) > for testing on the SDX platform. If you could do some testing in the meantime, that would be superb. Thanks for that in advance! Regards, Gabor
Hi Miquel, [...] >> --- >> Preferably, this should go in via the SPI tree along with the previous >> patch. It is not a strict requirement though, in the case it gets >> included separately through the mtd tree it reveals the bug fixed in >> the first patch. > > Sorry, didn't see that in the first place. Fine by me. Sorry for the inconvenience. Should I add these kind of notes into the cover letter next time? > >> --- >> drivers/mtd/nand/qpic_common.c | 28 ++++++++++++++++++++++++---- >> include/linux/mtd/nand-qpic-common.h | 8 ++++++++ >> 2 files changed, 32 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c >> index e0ed25b5afea9b289b767cd3d9c2d7572ed52008..fb1f81e4bdacaa3e81660a20e164926c64633513 100644 >> --- a/drivers/mtd/nand/qpic_common.c >> +++ b/drivers/mtd/nand/qpic_common.c >> @@ -15,6 +15,13 @@ >> #include <linux/slab.h> >> #include <linux/mtd/nand-qpic-common.h> >> >> +static inline int qcom_err_bam_array_full(struct qcom_nand_controller *nandc, >> + const char *name) >> +{ >> + dev_err(nandc->dev, "BAM %s array is full\n", name); >> + return -EINVAL; >> +} > > This is rather uncommon, I don't know if it's very relevant to do > that. Please drop this static inline function. The purpose of the inline function is a bit similar to dev_err_probe(). It helps to standardize the error message, and it allows to print the message and return with a value in one go. So this kind of statement ... 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; } ... can be simplied like this: if (bam_txn->bam_ce_pos + size > bam_txn->bam_ce_nitems) return qcom_err_bam_array_full(nandc, "CE"); The latter is cleaner for me, but no problem, I will remove that. Regards, Gabor
diff --git a/drivers/mtd/nand/qpic_common.c b/drivers/mtd/nand/qpic_common.c index e0ed25b5afea9b289b767cd3d9c2d7572ed52008..fb1f81e4bdacaa3e81660a20e164926c64633513 100644 --- a/drivers/mtd/nand/qpic_common.c +++ b/drivers/mtd/nand/qpic_common.c @@ -15,6 +15,13 @@ #include <linux/slab.h> #include <linux/mtd/nand-qpic-common.h> +static inline int qcom_err_bam_array_full(struct qcom_nand_controller *nandc, + const char *name) +{ + dev_err(nandc->dev, "BAM %s array is full\n", name); + return -EINVAL; +} + /** * qcom_free_bam_transaction() - Frees the BAM transaction memory * @nandc: qpic nand controller @@ -57,14 +64,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 +245,9 @@ 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) + return qcom_err_bam_array_full(nandc, "CE"); + bam_ce_buffer = &bam_txn->bam_ce[bam_txn->bam_ce_pos]; /* fill the command desc */ @@ -258,6 +269,9 @@ 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) + return qcom_err_bam_array_full(nandc, "CMD sgl"); + 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 +311,16 @@ 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) + return qcom_err_bam_array_full(nandc, "RX sgl"); + 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) + return qcom_err_bam_array_full(nandc, "TX sgl"); + 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;
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 with the 'spi-qpic-snand' driver only. Signed-off-by: Gabor Juhos <j4g8y7@gmail.com> --- Preferably, this should go in via the SPI tree along with the previous patch. It is not a strict requirement though, in the case it gets included separately through the mtd tree it reveals the bug fixed in the first patch. --- drivers/mtd/nand/qpic_common.c | 28 ++++++++++++++++++++++++---- include/linux/mtd/nand-qpic-common.h | 8 ++++++++ 2 files changed, 32 insertions(+), 4 deletions(-)