Re: [PATCH] iio: trigger: stm32-timer: fix the usage of uninitialized variables

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 10/1/19 10:43 AM, Jonathan Cameron wrote:
> On Mon, 30 Sep 2019 13:44:49 -0700
> Yizhuo <yzhai003@xxxxxxx> 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. This patch fixes most of the uninitialized
>> variables, but those in function stm32_tt_read_frequency() are
>> hard to handle and need extra effot.
>>
>> Signed-off-by: Yizhuo <yzhai003@xxxxxxx>
> Hi Yizhuo,
>
> This is a slightly interesting case.
>
>  From an 'obviously' correct point of view, it makes sense to
> always handle the errors and avoid the potentially uninitialised
> cases.
>
>  From the point of view of a 'fix' we need to show if these can
> actually happen.  These calls are regmap-mmio so the error
> path is in clock enable / disable if there is a clock provided.
> Looking at mfd/stm-timers.c there is a clock called "int" provided.
> Now it's possible that clock enable can never fail, but that is
> getting too hard to chase down.
>
> As such, I think this fix is worthwhile, but I'm not going to
> mark it for stable without a report of it actually being
> possible to trigger it.
>
> I'll leave this on the list for a little longer though as I'd definitely
> like an ack from Benjamin or someone else more familiar with the driver
> than I am.
>
> Thanks,
>
> Jonathan

Hi Yizhuo,

The only reason that could lead a regmap_read to failed is an issue on 
the clock
but is this case we will be blocked in regmap_read() itself so the error 
will never
be displayed.
For me testing all regmap_read() return values only add code and not 
bring additional informations
so I NACK this patch.

Benjamin

>> ---
>>   drivers/iio/trigger/stm32-timer-trigger.c | 98 ++++++++++++++++++++---
>>   1 file changed, 85 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/iio/trigger/stm32-timer-trigger.c b/drivers/iio/trigger/stm32-timer-trigger.c
>> index a5dfe65cd9b9..f8ea7bcbb739 100644
>> --- a/drivers/iio/trigger/stm32-timer-trigger.c
>> +++ b/drivers/iio/trigger/stm32-timer-trigger.c
>> @@ -107,6 +107,7 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>   	unsigned long long prd, div;
>>   	int prescaler = 0;
>>   	u32 ccer, cr1;
>> +	int ret;
>>   
>>   	/* Period and prescaler values depends of clock rate */
>>   	div = (unsigned long long)clk_get_rate(priv->clk);
>> @@ -132,11 +133,21 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>   	}
>>   
>>   	/* Check if nobody else use the timer */
>> -	regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +	if (ret) {
>> +		dev_err(priv->dev, "fail to read TIM_CCER.\n");
>> +		return ret;
>> +	}
>> +
>>   	if (ccer & TIM_CCER_CCXE)
>>   		return -EBUSY;
>>   
>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>> +	ret = regmap_read(priv->regmap, TIM_CR1, &cr1);
>> +	if (ret) {
>> +		dev_err(priv->dev, "fail to read TIM_CR1.\n");
>> +		return ret;
>> +	}
>> +
>>   	if (!(cr1 & TIM_CR1_CEN))
>>   		clk_enable(priv->clk);
>>   
>> @@ -164,12 +175,23 @@ static int stm32_timer_start(struct stm32_timer_trigger *priv,
>>   static void stm32_timer_stop(struct stm32_timer_trigger *priv)
>>   {
>>   	u32 ccer, cr1;
>> +	int ret;
>> +
>> +	ret = regmap_read(priv->regmap, TIM_CCER, &ccer);
>> +	if (ret) {
>> +		dev_err(priv->dev, "Fail to read TIM_CCER.\n");
>> +		return;
>> +	}
>>   
>> -	regmap_read(priv->regmap, TIM_CCER, &ccer);
>>   	if (ccer & TIM_CCER_CCXE)
>>   		return;
>>   
>> -	regmap_read(priv->regmap, TIM_CR1, &cr1);
>> +	ret = regmap_read(priv->regmap, TIM_CR1, &cr1);
>> +	if (ret) {
>> +		dev_err(priv->dev, "Fail to read TIM_CR1.\n");
>> +		return;
>> +	}
>> +
>>   	if (cr1 & TIM_CR1_CEN)
>>   		clk_disable(priv->clk);
>>   
>> @@ -403,20 +425,36 @@ static int stm32_counter_read_raw(struct iio_dev *indio_dev,
>>   {
>>   	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>   	u32 dat;
>> +	int ret;
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_RAW:
>> -		regmap_read(priv->regmap, TIM_CNT, &dat);
>> +		ret = regmap_read(priv->regmap, TIM_CNT, &dat);
>> +		if (ret) {
>> +			dev_err(priv->dev, "fail to read TIM_CNT.\n");
>> +			return ret;
>> +		}
>> +
>>   		*val = dat;
>>   		return IIO_VAL_INT;
>>   
>>   	case IIO_CHAN_INFO_ENABLE:
>> -		regmap_read(priv->regmap, TIM_CR1, &dat);
>> +		ret = regmap_read(priv->regmap, TIM_CR1, &dat);
>> +		if (ret) {
>> +			dev_err(priv->dev, "fail to read TIM_CR1.\n");
>> +			return ret;
>> +		}
>> +
>>   		*val = (dat & TIM_CR1_CEN) ? 1 : 0;
>>   		return IIO_VAL_INT;
>>   
>>   	case IIO_CHAN_INFO_SCALE:
>> -		regmap_read(priv->regmap, TIM_SMCR, &dat);
>> +		ret = regmap_read(priv->regmap, TIM_SMCR, &dat);
>> +		if (ret) {
>> +			dev_err(priv->dev, "fail to read TIM_SMCR.\n");
>> +			return ret;
>> +		}
>> +
>>   		dat &= TIM_SMCR_SMS;
>>   
>>   		*val = 1;
>> @@ -438,6 +476,7 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>   {
>>   	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>   	u32 dat;
>> +	int ret;
>>   
>>   	switch (mask) {
>>   	case IIO_CHAN_INFO_RAW:
>> @@ -449,13 +488,23 @@ static int stm32_counter_write_raw(struct iio_dev *indio_dev,
>>   
>>   	case IIO_CHAN_INFO_ENABLE:
>>   		if (val) {
>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>> +			ret = regmap_read(priv->regmap, TIM_CR1, &dat);
>> +			if (ret) {
>> +				dev_err(priv->dev, "fail to read TIM_CR1.\n");
>> +				return ret;
>> +			}
>> +
>>   			if (!(dat & TIM_CR1_CEN))
>>   				clk_enable(priv->clk);
>>   			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>   					   TIM_CR1_CEN);
>>   		} else {
>> -			regmap_read(priv->regmap, TIM_CR1, &dat);
>> +			ret = regmap_read(priv->regmap, TIM_CR1, &dat);
>> +			if (ret) {
>> +				dev_err(priv->dev, "fail to read TIM_CR1.\n");
>> +				return ret;
>> +			}
>> +
>>   			regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_CEN,
>>   					   0);
>>   			if (dat & TIM_CR1_CEN)
>> @@ -517,8 +566,13 @@ static int stm32_get_trigger_mode(struct iio_dev *indio_dev,
>>   {
>>   	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>   	u32 smcr;
>> +	int ret;
>>   
>> -	regmap_read(priv->regmap, TIM_SMCR, &smcr);
>> +	ret = regmap_read(priv->regmap, TIM_SMCR, &smcr);
>> +	if (ret) {
>> +		dev_err(priv->dev, "fail to read TIM_SMCR.\n");
>> +		return ret;
>> +	}
>>   
>>   	return (smcr & TIM_SMCR_SMS) == TIM_SMCR_SMS ? 0 : -EINVAL;
>>   }
>> @@ -557,6 +611,7 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>   	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>   	int sms = stm32_enable_mode2sms(mode);
>>   	u32 val;
>> +	int ret;
>>   
>>   	if (sms < 0)
>>   		return sms;
>> @@ -565,7 +620,12 @@ static int stm32_set_enable_mode(struct iio_dev *indio_dev,
>>   	 * enable counter clock, so it can use it. Keeps it in sync with CEN.
>>   	 */
>>   	if (sms == 6) {
>> -		regmap_read(priv->regmap, TIM_CR1, &val);
>> +		ret = regmap_read(priv->regmap, TIM_CR1, &val);
>> +		if (ret) {
>> +			dev_err(priv->dev, "fail to read TIM_CR1.\n");
>> +			return ret;
>> +		}
>> +
>>   		if (!(val & TIM_CR1_CEN))
>>   			clk_enable(priv->clk);
>>   	}
>> @@ -594,8 +654,14 @@ static int stm32_get_enable_mode(struct iio_dev *indio_dev,
>>   {
>>   	struct stm32_timer_trigger *priv = iio_priv(indio_dev);
>>   	u32 smcr;
>> +	int ret;
>> +
>> +	ret = regmap_read(priv->regmap, TIM_SMCR, &smcr);
>> +	if (ret) {
>> +		dev_err(priv->dev, "fail to read TIM_SMCR.\n");
>> +		return ret;
>> +	}
>>   
>> -	regmap_read(priv->regmap, TIM_SMCR, &smcr);
>>   	smcr &= TIM_SMCR_SMS;
>>   
>>   	return stm32_sms2enable_mode(smcr);
>> @@ -706,13 +772,19 @@ EXPORT_SYMBOL(is_stm32_timer_trigger);
>>   static void stm32_timer_detect_trgo2(struct stm32_timer_trigger *priv)
>>   {
>>   	u32 val;
>> +	int ret;
>>   
>>   	/*
>>   	 * Master mode selection 2 bits can only be written and read back when
>>   	 * timer supports it.
>>   	 */
>>   	regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS2, TIM_CR2_MMS2);
>> -	regmap_read(priv->regmap, TIM_CR2, &val);
>> +	ret = regmap_read(priv->regmap, TIM_CR2, &val);
>> +	if (ret) {
>> +		dev_err(priv->dev, "fail to read TIM_CR2.\n");
>> +		return;
>> +	}
>> +
>>   	regmap_update_bits(priv->regmap, TIM_CR2, TIM_CR2_MMS2, 0);
>>   	priv->has_trgo2 = !!val;
>>   }




[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux