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>
---
drivers/iio/adc/stm32-dfsdm-adc.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
@@ -1292,9 +1292,11 @@ static irqreturn_t stm32_dfsdm_irq(int irq, void *arg)structstm32_dfsdm_adc*adc=iio_priv(indio_dev);structregmap*regmap=adc->dfsdm->regmap;unsignedintstatus,int_en;+intret;-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)+returnIRQ_HANDLED;if(status&DFSDM_ISR_REOCF_MASK){/* Read the data register clean the IRQ status */
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.
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 hunk
+ 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 <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
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),
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:
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
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
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?
This one is still outstanding. If anyone from stm32 side of things could take a look
that would be great,
Jonathan
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),
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?
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
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),