| Message ID | 20190928002852.28329-1-yzhai003@ucr.edu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: adc: imx25-gcq: Variable could be uninitialized if regmap_read() fails | expand |
Hi Yizhuo, thanks for your patch. On 19-09-27 17:28, Yizhuo wrote: > In function mx25_gcq_irq(), local variable "stats" could > be uninitialized if function regmap_read() returns -EINVAL. > However, this value is used in if statement, which is > potentially unsafe. The same case applied to the variable > "data" in function mx25_gcq_get_raw_value() in the same file. IMHO the commit header should be something like: "iio: adc: imx25-gcq: fix uninitialized variable usage"... and please add a fixes tag. > > Signed-off-by: Yizhuo <yzhai003@ucr.edu> > --- > drivers/iio/adc/fsl-imx25-gcq.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c > index df19ecae52f7..dbf3e8e85aba 100644 > --- a/drivers/iio/adc/fsl-imx25-gcq.c > +++ b/drivers/iio/adc/fsl-imx25-gcq.c > @@ -74,7 +74,10 @@ static irqreturn_t mx25_gcq_irq(int irq, void *data) > struct mx25_gcq_priv *priv = data; > u32 stats; > > - regmap_read(priv->regs, MX25_ADCQ_SR, &stats); > + int ret = regmap_read(priv->regs, MX25_ADCQ_SR, &stats); Please don't do this. First declare the variable and then use it. Regards, Marco > + > + if (ret) > + return ret; > > if (stats & MX25_ADCQ_SR_EOQ) { > regmap_update_bits(priv->regs, MX25_ADCQ_MR, > @@ -121,7 +124,10 @@ static int mx25_gcq_get_raw_value(struct device *dev, > return -ETIMEDOUT; > } > > - regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); > + int ret = regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); > + > + if (ret) > + return ret; > > *val = MX25_ADCQ_FIFO_DATA(data); > > -- > 2.17.1 > > >
On Mon, 30 Sep 2019 09:44:12 +0200 Marco Felsch <m.felsch@pengutronix.de> wrote: > Hi Yizhuo, > > thanks for your patch. > > On 19-09-27 17:28, Yizhuo wrote: > > In function mx25_gcq_irq(), local variable "stats" could > > be uninitialized if function regmap_read() returns -EINVAL. > > However, this value is used in if statement, which is > > potentially unsafe. The same case applied to the variable > > "data" in function mx25_gcq_get_raw_value() in the same file. > > IMHO the commit header should be something like: "iio: adc: imx25-gcq: > fix uninitialized variable usage"... > > and please add a fixes tag. As with the others, before adding a fixes tag, please verify there is an actual path to trigger this. In this case it's an mmio regmap with no clock. For those, I'm not sure if there is a failure path. Still a worthwhile hardening / cleanup patch, but shouldn't be called a fix or marked with a fixes tag because we don't want people to think it is necessary to backport it. Thanks, Jonathan > > > > > Signed-off-by: Yizhuo <yzhai003@ucr.edu> > > --- > > drivers/iio/adc/fsl-imx25-gcq.c | 10 ++++++++-- > > 1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c > > index df19ecae52f7..dbf3e8e85aba 100644 > > --- a/drivers/iio/adc/fsl-imx25-gcq.c > > +++ b/drivers/iio/adc/fsl-imx25-gcq.c > > @@ -74,7 +74,10 @@ static irqreturn_t mx25_gcq_irq(int irq, void *data) > > struct mx25_gcq_priv *priv = data; > > u32 stats; > > > > - regmap_read(priv->regs, MX25_ADCQ_SR, &stats); > > + int ret = regmap_read(priv->regs, MX25_ADCQ_SR, &stats); > > Please don't do this. First declare the variable and then use it. > > Regards, > Marco > > > + > > + if (ret) > > + return ret; > > > > if (stats & MX25_ADCQ_SR_EOQ) { > > regmap_update_bits(priv->regs, MX25_ADCQ_MR, > > @@ -121,7 +124,10 @@ static int mx25_gcq_get_raw_value(struct device *dev, > > return -ETIMEDOUT; > > } > > > > - regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); > > + int ret = regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); > > + > > + if (ret) > > + return ret; > > > > *val = MX25_ADCQ_FIFO_DATA(data); > > > > -- > > 2.17.1 > > > > > > >
diff --git a/drivers/iio/adc/fsl-imx25-gcq.c b/drivers/iio/adc/fsl-imx25-gcq.c index df19ecae52f7..dbf3e8e85aba 100644 --- a/drivers/iio/adc/fsl-imx25-gcq.c +++ b/drivers/iio/adc/fsl-imx25-gcq.c @@ -74,7 +74,10 @@ static irqreturn_t mx25_gcq_irq(int irq, void *data) struct mx25_gcq_priv *priv = data; u32 stats; - regmap_read(priv->regs, MX25_ADCQ_SR, &stats); + int ret = regmap_read(priv->regs, MX25_ADCQ_SR, &stats); + + if (ret) + return ret; if (stats & MX25_ADCQ_SR_EOQ) { regmap_update_bits(priv->regs, MX25_ADCQ_MR, @@ -121,7 +124,10 @@ static int mx25_gcq_get_raw_value(struct device *dev, return -ETIMEDOUT; } - regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); + int ret = regmap_read(priv->regs, MX25_ADCQ_FIFO, &data); + + if (ret) + return ret; *val = MX25_ADCQ_FIFO_DATA(data);
In function mx25_gcq_irq(), local variable "stats" could be uninitialized if function regmap_read() returns -EINVAL. However, this value is used in if statement, which is potentially unsafe. The same case applied to the variable "data" in function mx25_gcq_get_raw_value() in the same file. Signed-off-by: Yizhuo <yzhai003@ucr.edu> --- drivers/iio/adc/fsl-imx25-gcq.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)