Message ID | 20190929164848.13930-1-yzhai003@ucr.edu (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | iio: adc: meson-saradc: Variables could be uninitalized if regmap_read() fails | expand |
Hi Yizhuo, thank you for this patch On Sun, Sep 29, 2019 at 6:48 PM Yizhuo <yzhai003@ucr.edu> 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 <yzhai003@ucr.edu> > --- > 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 = 10000; > + 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 = 10000; > @@ -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, &val); > + ret = regmap_read(priv->regmap, > + MESON_SAR_ADC_DELAY, &val); > + if (ret) > + return ret; this is a big problem because we're returning with &indio_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, ®val); > + 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
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 = 10000; + 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, ®val); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ®val); + 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(&priv->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, ®val); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_FIFO_RD, ®val); + if (ret) + return ret; + fifo_chan = FIELD_GET(MESON_SAR_ADC_FIFO_RD_CHAN_ID_MASK, regval); if (fifo_chan != chan->address) { dev_err(&indio_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 = 10000; + int ret; mutex_lock(&indio_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, &val); + ret = regmap_read(priv->regmap, + MESON_SAR_ADC_DELAY, &val); + 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, ®val); + ret = regmap_read(priv->regmap, MESON_SAR_ADC_REG3, ®val); + 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, ®val); + if (ret) + return ret; - regmap_read(priv->regmap, MESON_SAR_ADC_REG0, ®val); cnt = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_COUNT_MASK, regval); threshold = FIELD_GET(MESON_SAR_ADC_REG0_FIFO_CNT_IRQ_MASK, regval);
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 <yzhai003@ucr.edu> --- drivers/iio/adc/meson_saradc.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)