| Message ID | 20210719195313.40341-1-yzhai003@ucr.edu (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: adc: stm32-dfsdm: Fix the uninitialized use if regmap_read() fails | expand |
On Mon, 19 Jul 2021 19:53:11 +0000 Yizhuo <yzhai003@ucr.edu> wrote: > 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 <yzhai003@ucr.edu> 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. > --- > 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 > + 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),
On Sat, 24 Jul 2021 16:48:40 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 19 Jul 2021 19:53:11 +0000 > Yizhuo <yzhai003@ucr.edu> wrote: > > > 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 <yzhai003@ucr.edu> > > 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 > > > --- > > 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 > > > + 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), >
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 <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 On Sun, Aug 8, 2021 at 10:29 AM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sat, 24 Jul 2021 16:48:40 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Mon, 19 Jul 2021 19:53:11 +0000 > > Yizhuo <yzhai003@ucr.edu> wrote: > > > > > 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 <yzhai003@ucr.edu> > > > > 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 > > > > > > --- > > > 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 > > > > > + 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
On Sun, 8 Aug 2021 18:32:43 +0100 Jonathan Cameron <jic23@kernel.org> wrote: > On Sat, 24 Jul 2021 16:48:40 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Mon, 19 Jul 2021 19:53:11 +0000 > > Yizhuo <yzhai003@ucr.edu> wrote: > > > > > 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 <yzhai003@ucr.edu> > > > > 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? This one is still outstanding. If anyone from stm32 side of things could take a look that would be great, Jonathan > > Thanks, > > Jonathan > > > > > > --- > > > 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 > > > > > + 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), > > >
Hi, On 10/3/21 5:47 PM, Jonathan Cameron wrote: > On Sun, 8 Aug 2021 18:32:43 +0100 > Jonathan Cameron <jic23@kernel.org> wrote: > >> On Sat, 24 Jul 2021 16:48:40 +0100 >> Jonathan Cameron <jic23@kernel.org> wrote: >> >>> On Mon, 19 Jul 2021 19:53:11 +0000 >>> Yizhuo <yzhai003@ucr.edu> wrote: >>> >>>> 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 <yzhai003@ucr.edu> >>> >>> 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? > > This one is still outstanding. If anyone from stm32 side of things could take a look > that would be great, > > Jonathan > I cannot see side effects with reordering itself. However, if we get an error with the read access, just leaving with irq_handled status is probably not enough. In such case we are facing a serious issue and it would make sense to return irq_none instead, as the interrupt will probably never be acknowledged. BRs >> >> Thanks, >> >> Jonathan >> >>> >>>> --- >>>> 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 >>> >>>> + 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), >>> >> >
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); + 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),
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 <yzhai003@ucr.edu> --- drivers/iio/adc/stm32-dfsdm-adc.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)