[PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[Yizhuo]

Inside function stm32_pwm_config(), variable "psc" and " arr"
could be uninitialized if regmap_read() returns -EINVALs.
However, they are used later in the if statement to decide
the return value which is potentially unsafe.

The same case happens in function stm32_pwm_detect_channels()
with variable "ccer", but we cannot just return -EINVAL because
the error code is not acceptable by the caller. Aslo, the variable
"ccer" in functionstm32_pwm_detect_complementary() could also be
uninitialized, since stm32_pwm_detect_complementary() returns void,
the patch is not easy.

Signed-off-by: Yizhuo <yzhai003@ucr.edu>
---
 drivers/pwm/pwm-stm32.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-stm32.c b/drivers/pwm/pwm-stm32.c
index 359b08596d9e..22c54df52977 100644
--- a/drivers/pwm/pwm-stm32.c
+++ b/drivers/pwm/pwm-stm32.c
@@ -346,9 +346,15 @@ static int stm32_pwm_config(struct stm32_pwm *priv, int ch,
 	 */
 	if (active_channels(priv) & ~(1 << ch * 4)) {
 		u32 psc, arr;
+		int ret;
 
-		regmap_read(priv->regmap, TIM_PSC, &psc);
-		regmap_read(priv->regmap, TIM_ARR, &arr);
+		ret = regmap_read(priv->regmap, TIM_PSC, &psc);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(priv->regmap, TIM_ARR, &arr);
+		if (ret)
+			return ret;
 
 		if ((psc != prescaler) || (arr != prd - 1))
 			return -EBUSY;
-- 
2.17.1

Re: [PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[Uwe Kleine-König]

Hello,

On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote:
> Inside function stm32_pwm_config(), variable "psc" and " arr"
> could be uninitialized if regmap_read() returns -EINVALs.
> However, they are used later in the if statement to decide
> the return value which is potentially unsafe.
> 
> The same case happens in function stm32_pwm_detect_channels()
> with variable "ccer", but we cannot just return -EINVAL because
> the error code is not acceptable by the caller. Aslo, the variable

s/Aslo/Also/

> "ccer" in functionstm32_pwm_detect_complementary() could also be

s/functionstm32_pwm_detect_/function stm32_pwm_detect_/

> uninitialized, since stm32_pwm_detect_complementary() returns void,
> the patch is not easy.

active_channels() is also affected. Also there are calls to
regmap_update_bits which should have their return values checked.

While a patch to fix these all is not trivial it is certainly possible
and I would prefer to fix the problem completely.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [Linux-stm32] [PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[Benjamin GAIGNARD]

On 10/4/19 8:23 AM, Uwe Kleine-König wrote:
> Hello,
>
> On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote:
>> Inside function stm32_pwm_config(), variable "psc" and " arr"
>> could be uninitialized if regmap_read() returns -EINVALs.
>> However, they are used later in the if statement to decide
>> the return value which is potentially unsafe.

Hi Yizhuo,

like for the your patch in IIO trigger regmap_read could only failed
if the hardware block is no more clocked and in this case we won't 
return of regmap_read.
Testing regmap_read() return value just add code but doesn't provide a 
valid information.
If you really want to log all the possible errors cases please do it in 
regmap code itself and
not in *all* the drivers.

Thanks,

Benjamin

>>
>> The same case happens in function stm32_pwm_detect_channels()
>> with variable "ccer", but we cannot just return -EINVAL because
>> the error code is not acceptable by the caller. Aslo, the variable
> s/Aslo/Also/
>
>> "ccer" in functionstm32_pwm_detect_complementary() could also be
> s/functionstm32_pwm_detect_/function stm32_pwm_detect_/
>
>> uninitialized, since stm32_pwm_detect_complementary() returns void,
>> the patch is not easy.
> active_channels() is also affected. Also there are calls to
> regmap_update_bits which should have their return values checked.
>
> While a patch to fix these all is not trivial it is certainly possible
> and I would prefer to fix the problem completely.
>
> Best regards
> Uwe
>

Re: [Linux-stm32] [PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[Uwe Kleine-König]

Hello,

Cc += Mark Brown who maintains regmap

On Fri, Oct 04, 2019 at 09:09:51AM +0000, Benjamin GAIGNARD wrote:
> 
> On 10/4/19 8:23 AM, Uwe Kleine-König wrote:
> > Hello,
> >
> > On Thu, Oct 03, 2019 at 09:46:49PM -0700, Yizhuo wrote:
> >> Inside function stm32_pwm_config(), variable "psc" and " arr"
> >> could be uninitialized if regmap_read() returns -EINVALs.
> >> However, they are used later in the if statement to decide
> >> the return value which is potentially unsafe.
> 
> Hi Yizhuo,
> 
> like for the your patch in IIO trigger regmap_read could only failed
> if the hardware block is no more clocked and in this case we won't 
> return of regmap_read.

I'm not sure this is aligned with how regmap is supposed to be used. I
think the driver making use of regmap is not supposed to make any
assumptions about how and when a read or write access can or cannot fail
and instead is supposed to check all return values. So IMHO the patch
goes in the right direction.

> Testing regmap_read() return value just add code but doesn't provide a 
> valid information.
> If you really want to log all the possible errors cases please do it in 
> regmap code itself and
> not in *all* the drivers.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [Linux-stm32] [PATCH] pwm: stm32: Fix the usage of uninitialized variable in stm32_pwm_config()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1173 bytes --]

On Fri, Oct 04, 2019 at 10:08:04PM +0200, Uwe Kleine-König wrote:
> On Fri, Oct 04, 2019 at 09:09:51AM +0000, Benjamin GAIGNARD wrote:

> > like for the your patch in IIO trigger regmap_read could only failed
> > if the hardware block is no more clocked and in this case we won't 
> > return of regmap_read.

> I'm not sure this is aligned with how regmap is supposed to be used. I
> think the driver making use of regmap is not supposed to make any
> assumptions about how and when a read or write access can or cannot fail
> and instead is supposed to check all return values. So IMHO the patch
> goes in the right direction.

Yeah, if you're being very good about checking return values you really
should check them all the time in case something comes up - you could
get errors that don't come from the physical access, for example on read
if a register is marked as unreadable, or if physical access is required
but the map has been marked cache only.  That said a lot of people will
just not check anything since it's not common to see errors and errors
that do occur tend to have other quite visible effects like I2C bus
errors or system hangs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]