Re: [PATCH] iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails
Hi Yizhuo, thank you for this patch On Sun, Sep 29, 2019 at 6:48 PM Yizhuo wrote: > > Several functions in this file are trying to use regmap_read() to > initialize the specific variable, however, if regmap_read() fails, > the variable could be uninitialized but used directly, which is > potentially unsafe. The return value of regmap_read() should be > checked and handled. the meson_saradc driver is using a MMIO regmap so read failures are unlikely (unless there's a bug in the code somewhere) did you find these issues with some static code analysis tool or did you experience a real world problem? > Signed-off-by: Yizhuo > --- > drivers/iio/adc/meson_saradc.c | 30 -- > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > index 7b28d045d271..4b6c2983ef39 100644 > --- a/drivers/iio/adc/meson_saradc.c > +++ b/drivers/iio/adc/meson_saradc.c > @@ -323,6 +323,7 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev > *indio_dev) > { > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > int regval, timeout = 1; > + int ret; why not add "ret" to the variables in the line above? so this could be shortened to (which is also consistent with the coding style in meson_saradc): int ret, regval, timeout = 1; > @@ -506,7 +514,10 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) > */ > do { > udelay(1); > - regmap_read(priv->regmap, MESON_SAR_ADC_DELAY, ); > + ret = regmap_read(priv->regmap, > + MESON_SAR_ADC_DELAY, ); > + if (ret) > + return ret; this is a big problem because we're returning with _dev->mlock held see the "timeout < 0" block below > @@ -771,7 +782,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev) > { > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > int regval, i, ret; > - > + int ret; this removes an empty line between the variable declarations and the first code (comment) also "ret" is already defined in the line before (I haven't compile-tested this myself yet but I'm surprised that this doesn't give an error or at least a warning) > @@ -1013,8 +1027,12 @@ static irqreturn_t meson_sar_adc_irq(int irq, void > *data) > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > unsigned int cnt, threshold; > u32 regval; > + int ret; > + > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > + if (ret) > + return ret; this function doesn't return "int" but irqreturn_t the only allowed values are: [0] Martin [0] https://elixir.bootlin.com/linux/v5.3/source/include/linux/irqreturn.h#L11
Re: [PATCH] iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails
Hi Nicholas: Thanks for your feedback, I made changes and submitted new patches already. Check patch script generated a warning if I strictly align the parameter with (. I checked other code inside this file and modified the continuation accordingly. On Fri, Sep 27, 2019 at 11:47 PM Nicholas Mc Guire wrote: > > On Fri, Sep 27, 2019 at 05:46:41PM -0700, Yizhuo wrote: > > Several functions in this file are trying to use regmap_read() to > > initialize the specific variable, however, if regmap_read() fails, > > the variable could be uninitialized but used directly, which is > > potentially unsafe. The return value of regmap_read() should be > > checked and handled. > > > > Signed-off-by: Yizhuo > > Just a few minor style issues - contentwise it look correct to me. > Reviewed-by: Nicholas Mc Guire > > > --- > > drivers/iio/adc/meson_saradc.c | 28 +++- > > 1 file changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > > index 7b28d045d271..c032a64108b4 100644 > > --- a/drivers/iio/adc/meson_saradc.c > > +++ b/drivers/iio/adc/meson_saradc.c > > @@ -323,6 +323,7 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev > > *indio_dev) > > { > > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > > int regval, timeout = 1; > > + int ret; > > > > /* > >* NOTE: we need a small delay before reading the status, otherwise > > @@ -331,7 +332,9 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev > > *indio_dev) > >*/ > > do { > > udelay(1); > > - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > > + if (ret) > > + return ret; > > } while (FIELD_GET(MESON_SAR_ADC_REG0_BUSY_MASK, regval) && > > timeout--); > > > > if (timeout < 0) > > @@ -358,7 +361,11 @@ static int meson_sar_adc_read_raw_sample(struct > > iio_dev *indio_dev, > > any reason not to declear ret in the declaration block ? > so just for consistency with coding style within meson_saradc.c > this might be: > > int regval, fifo_chan, fifo_val, count; > + int ret; > > > return -EINVAL; > > } > > > > - regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); > > + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); > > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); > > > + > > + if (ret) > > + return ret; > > + > > fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval); > > if (fifo_chan != chan->address) { > > dev_err(_dev->dev, > > @@ -491,6 +498,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) > > { > > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > > int val, timeout = 1; > > + int ret; > > > > mutex_lock(_dev->mlock); > > > > @@ -506,7 +514,10 @@ static int meson_sar_adc_lock(struct iio_dev > > *indio_dev) > >*/ > > do { > > udelay(1); > > - regmap_read(priv->regmap, MESON_SAR_ADC_DELAY, ); > > + ret = regmap_read(priv->regmap, > > + MESON_SAR_ADC_DELAY, ); > > checkpatch does not fuss here but the continuation should be alligned > witht the ( here > > > + if (ret) > > + return ret; > > } while (val & MESON_SAR_ADC_DELAY_BL30_BUSY && timeout--); > > > > if (timeout < 0) { > > @@ -784,7 +795,10 @@ static int meson_sar_adc_init(struct iio_dev > > *indio_dev) > >* BL30 to make sure BL30 gets the values it expects when > >* reading the temperature sensor. > >*/ > > - regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); > > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); > > + if (ret) > > + return ret; > > + > > if (regval & MESON_SAR_ADC_REG3_BL30_INITIALIZED) > > return 0; > > } > > @@ -1014,7 +1028,11 @@ static irqreturn_t meson_sar_adc_irq(int irq, void > > *data) > > unsigned int cnt, threshold; > > u32 regval; > > same as above > > + int ret; > > > > > - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > > + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > > > + > > + if (ret) > > + return ret; > > + > > cnt = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_COUNT_MASK, regval); > > threshold = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); > > > > -- > > 2.17.1 > > -- Kind Regards, Yizhuo Zhai Computer Science, Graduate Student University of California, Riverside
[PATCH] iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails
Several functions in this file are trying to use regmap_read() to initialize the specific variable, however, if regmap_read() fails, the variable could be uninitialized but used directly, which is potentially unsafe. The return value of regmap_read() should be checked and handled. Signed-off-by: Yizhuo --- drivers/iio/adc/meson_saradc.c | 30 -- 1 file changed, 24 insertions(+), 6 deletions(-) diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c index 7b28d045d271..4b6c2983ef39 100644 --- a/drivers/iio/adc/meson_saradc.c +++ b/drivers/iio/adc/meson_saradc.c @@ -323,6 +323,7 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev *indio_dev) { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int regval, timeout = 1; + int ret; /* * NOTE: we need a small delay before reading the status, otherwise @@ -331,7 +332,9 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev *indio_dev) */ do { udelay(1); - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + if (ret) + return ret; } while (FIELD_GET(MESON_SAR_ADC_REG0_BUSY_MASK, regval) && timeout--); if (timeout < 0) @@ -346,6 +349,7 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int regval, fifo_chan, fifo_val, count; + int ret; if(!wait_for_completion_timeout(>done, msecs_to_jiffies(MESON_SAR_ADC_TIMEOUT))) @@ -358,7 +362,10 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, return -EINVAL; } - regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); + if (ret) + return ret; + fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval); if (fifo_chan != chan->address) { dev_err(_dev->dev, @@ -491,6 +498,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int val, timeout = 1; + int ret; mutex_lock(_dev->mlock); @@ -506,7 +514,10 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) */ do { udelay(1); - regmap_read(priv->regmap, MESON_SAR_ADC_DELAY, ); + ret = regmap_read(priv->regmap, + MESON_SAR_ADC_DELAY, ); + if (ret) + return ret; } while (val & MESON_SAR_ADC_DELAY_BL30_BUSY && timeout--); if (timeout < 0) { @@ -771,7 +782,7 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev) { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int regval, i, ret; - + int ret; /* * make sure we start at CH7 input since the other muxes are only used * for internal calibration. @@ -784,7 +795,10 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev) * BL30 to make sure BL30 gets the values it expects when * reading the temperature sensor. */ - regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); + if (ret) + return ret; + if (regval & MESON_SAR_ADC_REG3_BL30_INITIALIZED) return 0; } @@ -1013,8 +1027,12 @@ static irqreturn_t meson_sar_adc_irq(int irq, void *data) struct meson_sar_adc_priv *priv = iio_priv(indio_dev); unsigned int cnt, threshold; u32 regval; + int ret; + + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + if (ret) + return ret; - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); cnt = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_COUNT_MASK, regval); threshold = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); -- 2.17.1
Re: [PATCH] iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails
On Fri, Sep 27, 2019 at 05:46:41PM -0700, Yizhuo wrote: > Several functions in this file are trying to use regmap_read() to > initialize the specific variable, however, if regmap_read() fails, > the variable could be uninitialized but used directly, which is > potentially unsafe. The return value of regmap_read() should be > checked and handled. > > Signed-off-by: Yizhuo Just a few minor style issues - contentwise it look correct to me. Reviewed-by: Nicholas Mc Guire > --- > drivers/iio/adc/meson_saradc.c | 28 +++- > 1 file changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c > index 7b28d045d271..c032a64108b4 100644 > --- a/drivers/iio/adc/meson_saradc.c > +++ b/drivers/iio/adc/meson_saradc.c > @@ -323,6 +323,7 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev > *indio_dev) > { > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > int regval, timeout = 1; > + int ret; > > /* >* NOTE: we need a small delay before reading the status, otherwise > @@ -331,7 +332,9 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev > *indio_dev) >*/ > do { > udelay(1); > - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > + if (ret) > + return ret; > } while (FIELD_GET(MESON_SAR_ADC_REG0_BUSY_MASK, regval) && timeout--); > > if (timeout < 0) > @@ -358,7 +361,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev > *indio_dev, any reason not to declear ret in the declaration block ? so just for consistency with coding style within meson_saradc.c this might be: int regval, fifo_chan, fifo_val, count; + int ret; > return -EINVAL; > } > > - regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); > + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); > + > + if (ret) > + return ret; > + > fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval); > if (fifo_chan != chan->address) { > dev_err(_dev->dev, > @@ -491,6 +498,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) > { > struct meson_sar_adc_priv *priv = iio_priv(indio_dev); > int val, timeout = 1; > + int ret; > > mutex_lock(_dev->mlock); > > @@ -506,7 +514,10 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) >*/ > do { > udelay(1); > - regmap_read(priv->regmap, MESON_SAR_ADC_DELAY, ); > + ret = regmap_read(priv->regmap, > + MESON_SAR_ADC_DELAY, ); checkpatch does not fuss here but the continuation should be alligned witht the ( here > + if (ret) > + return ret; > } while (val & MESON_SAR_ADC_DELAY_BL30_BUSY && timeout--); > > if (timeout < 0) { > @@ -784,7 +795,10 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev) >* BL30 to make sure BL30 gets the values it expects when >* reading the temperature sensor. >*/ > - regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); > + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); > + if (ret) > + return ret; > + > if (regval & MESON_SAR_ADC_REG3_BL30_INITIALIZED) > return 0; > } > @@ -1014,7 +1028,11 @@ static irqreturn_t meson_sar_adc_irq(int irq, void > *data) > unsigned int cnt, threshold; > u32 regval; same as above + int ret; > > - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); > + > + if (ret) > + return ret; > + > cnt = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_COUNT_MASK, regval); > threshold = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); > > -- > 2.17.1 >
[PATCH] iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails
Several functions in this file are trying to use regmap_read() to initialize the specific variable, however, if regmap_read() fails, the variable could be uninitialized but used directly, which is potentially unsafe. The return value of regmap_read() should be checked and handled. Signed-off-by: Yizhuo --- drivers/iio/adc/meson_saradc.c | 28 +++- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/drivers/iio/adc/meson_saradc.c b/drivers/iio/adc/meson_saradc.c index 7b28d045d271..c032a64108b4 100644 --- a/drivers/iio/adc/meson_saradc.c +++ b/drivers/iio/adc/meson_saradc.c @@ -323,6 +323,7 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev *indio_dev) { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int regval, timeout = 1; + int ret; /* * NOTE: we need a small delay before reading the status, otherwise @@ -331,7 +332,9 @@ static int meson_sar_adc_wait_busy_clear(struct iio_dev *indio_dev) */ do { udelay(1); - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + if (ret) + return ret; } while (FIELD_GET(MESON_SAR_ADC_REG0_BUSY_MASK, regval) && timeout--); if (timeout < 0) @@ -358,7 +361,11 @@ static int meson_sar_adc_read_raw_sample(struct iio_dev *indio_dev, return -EINVAL; } - regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ); + + if (ret) + return ret; + fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval); if (fifo_chan != chan->address) { dev_err(_dev->dev, @@ -491,6 +498,7 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) { struct meson_sar_adc_priv *priv = iio_priv(indio_dev); int val, timeout = 1; + int ret; mutex_lock(_dev->mlock); @@ -506,7 +514,10 @@ static int meson_sar_adc_lock(struct iio_dev *indio_dev) */ do { udelay(1); - regmap_read(priv->regmap, MESON_SAR_ADC_DELAY, ); + ret = regmap_read(priv->regmap, + MESON_SAR_ADC_DELAY, ); + if (ret) + return ret; } while (val & MESON_SAR_ADC_DELAY_BL30_BUSY && timeout--); if (timeout < 0) { @@ -784,7 +795,10 @@ static int meson_sar_adc_init(struct iio_dev *indio_dev) * BL30 to make sure BL30 gets the values it expects when * reading the temperature sensor. */ - regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ); + if (ret) + return ret; + if (regval & MESON_SAR_ADC_REG3_BL30_INITIALIZED) return 0; } @@ -1014,7 +1028,11 @@ static irqreturn_t meson_sar_adc_irq(int irq, void *data) unsigned int cnt, threshold; u32 regval; - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + int ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ); + + if (ret) + return ret; + cnt = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_COUNT_MASK, regval); threshold = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval); -- 2.17.1