Uninitialized Variable Use in drivers/iio/adc/stm32-dfsdm-adc.c

Uninitialized Variable Use in drivers/iio/adc/stm32-dfsdm-adc.c

[Yizhuo Zhai]

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
the error code directly. Could you please advise me here?

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

Re: Uninitialized Variable Use in drivers/iio/adc/stm32-dfsdm-adc.c

[Alexandru Ardelean]

On Mon, Jul 19, 2021 at 2:39 AM Yizhuo Zhai <yzhai003@ucr.edu> wrote:
>
> 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?

> 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.

>
> 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

Re: Uninitialized Variable Use in drivers/iio/adc/stm32-dfsdm-adc.c

[Yizhuo Zhai]

Alexandru:
Thanks for your detailed explanation, I would submit the patch accordingly.

>>Just curious: are you seeing any issues with these variables being uninitialized?
This bug is reported by the static analysis tool, we haven't
dynamically triggered it.


On Mon, Jul 19, 2021 at 12:47 AM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Mon, Jul 19, 2021 at 2:39 AM Yizhuo Zhai <yzhai003@ucr.edu> wrote:
> >
> > 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?
>
> > 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.
>
> >
> > 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



-- 
Kind Regards,

Yizhuo Zhai

Computer Science, Graduate Student
University of California, Riverside