Re: [PATCH] iio: adc: stm32-dfsdm: Fix the uninitialized use if regmap_read() fails

Hi Jonathan:
Thanks for your effort, and yes please take a look just in case.
FYI, I made the modifications align with Alexandru's help, this is the
original reply:

On Mon, Jul 19, 2021 at 12:47 AM Alexandru Ardelean
[off-list ref] wrote:

On Mon, Jul 19, 2021 at 2:39 AM Yizhuo Zhai [off-list ref] wrote:

quoted

Hi All:
Inside function stm32_dfsdm_irq(), the variable "status", "int_en"
could be uninitialized if the regmap_read() fails and returns an error
code.  However, they are directly used in the later context to decide
the control flow, which is potentially unsafe. However,
stm32_dfsdm_irq() returns the type irqreturn_t and I could not return

Just curious: are you seeing any issues with these variables being
uninitialized?

quoted

the error code directly. Could you please advise me here?

The correct way to do it, would be:

ret = regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
if (ret)
    return IRQ_HANDLED;

IRQ handlers should return one of
enum irqreturn {
    IRQ_NONE        = (0 << 0),
    IRQ_HANDLED     = (1 << 0),
    IRQ_WAKE_THREAD     = (1 << 1),
};

If you want to fully optimize/correct this, then it may be something like:

        ret = regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
        if (ret)
                return IRQ_HANDLED;

        if (status & DFSDM_ISR_REOCF_MASK) {
                /* Read the data register clean the IRQ status */
                regmap_read(regmap, DFSDM_RDATAR(adc->fl_id), adc->buffer);

// in this point, we could check for regmap_read(), but it won't make
sense; we should call the complete() handler, either way

                complete(&adc->completion);
        }

        if (status & DFSDM_ISR_ROVRF_MASK) {
                ret = regmap_read(regmap, DFSDM_CR2(adc->fl_id), &int_en);
                if (ret)
                        return IRQ_HANDLED;
                if (int_en & DFSDM_CR2_ROVRIE_MASK)
                        dev_warn(&indio_dev->dev, "Overrun detected\n");
                regmap_update_bits(regmap, DFSDM_ICR(adc->fl_id),
                                   DFSDM_ICR_CLRROVRF_MASK,
                                   DFSDM_ICR_CLRROVRF_MASK);

// in this point, we could also check the ret code; but we still need
to call IRQ_HANDLED anyway;
        }


Quite often, when regmap_read() returns errors, then something is
seriously wrong in the system.
Something else would usually fail or crash worse than this interrupt handler.
That being said, properly handling regmap_read() here is a good idea.

quoted

The related code:

static irqreturn_t stm32_dfsdm_irq(int irq, void *arg) {
    unsigned int status, int_en;

    regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
    regmap_read(regmap, DFSDM_CR2(adc->fl_id), &int_en);

    if (status & DFSDM_ISR_REOCF_MASK) {}
    if (status & DFSDM_ISR_ROVRF_MASK) {}
}


--
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

On Sun, Aug 8, 2021 at 10:29 AM Jonathan Cameron [off-list ref] wrote:

On Sat, 24 Jul 2021 16:48:40 +0100
Jonathan Cameron [off-list ref] wrote:

quoted

On Mon, 19 Jul 2021 19:53:11 +0000
Yizhuo [off-list ref] wrote:

quoted

Inside function stm32_dfsdm_irq(), the variable "status", "int_en"
could be uninitialized if the regmap_read() fails and returns an error
code.  However, they are directly used in the later context to decide
the control flow, which is potentially unsafe.

Fixes: e2e6771c64625 ("IIO: ADC: add STM32 DFSDM sigma delta ADC support")

Signed-off-by: Yizhuo <redacted>

Hi Yizhou

I want to get some review of this from people familiar with the
hardware as there is a small possibility your reordering might have
introduced a problem.

To stm32 people, can someone take a look at this?

Thanks,

Jonathan

quoted

quoted

---
 drivers/iio/adc/stm32-dfsdm-adc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/stm32-dfsdm-adc.c b/drivers/iio/adc/stm32-dfsdm-adc.c
index 1cfefb3b5e56..d8b78aead942 100644
--- a/drivers/iio/adc/stm32-dfsdm-adc.c
+++ b/drivers/iio/adc/stm32-dfsdm-adc.c

@@ -1292,9 +1292,11 @@ static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
    struct stm32_dfsdm_adc *adc = iio_priv(indio_dev);
    struct regmap *regmap = adc->dfsdm->regmap;
    unsigned int status, int_en;
+   int ret;

-   regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
-   regmap_read(regmap, DFSDM_CR2(adc->fl_id), &int_en);

Moving this later is only valid if there aren't any side effects.
The current ordering is strange enough it makes me wonder if there might be!

Jonathan

quoted

+   ret = regmap_read(regmap, DFSDM_ISR(adc->fl_id), &status);
+   if (ret)
+           return IRQ_HANDLED;

    if (status & DFSDM_ISR_REOCF_MASK) {
            /* Read the data register clean the IRQ status */

@@ -1303,6 +1305,9 @@ static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)
    }

    if (status & DFSDM_ISR_ROVRF_MASK) {
+           ret = regmap_read(regmap, DFSDM_CR2(adc->fl_id), &int_en);
+           if (ret)
+                   return IRQ_HANDLED;
            if (int_en & DFSDM_CR2_ROVRIE_MASK)
                    dev_warn(&indio_dev->dev, "Overrun detected\n");
            regmap_update_bits(regmap, DFSDM_ICR(adc->fl_id),

--
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel