[PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[Fabrice Gasnier]

The ceiling value isn't checked before writing it into registers. The user
could write a value higher than the counter resolution (e.g. 16 or 32 bits
indicated by max_arr). This makes most significant bits to be truncated.
Fix it by checking the max_arr to report a range error [1] to the user.

Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")

[1] https://lkml.org/lkml/2021/2/12/358

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
---
 drivers/counter/stm32-timer-cnt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
index ef2a974..2cf0c05 100644
--- a/drivers/counter/stm32-timer-cnt.c
+++ b/drivers/counter/stm32-timer-cnt.c
@@ -32,6 +32,7 @@ struct stm32_timer_cnt {
 	struct regmap *regmap;
 	struct clk *clk;
 	u32 ceiling;
+	u32 max_arr;
 	bool enabled;
 	struct stm32_timer_regs bak;
 };
@@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter,
 	if (ret)
 		return ret;
 
+	if (ceiling > priv->max_arr)
+		return -ERANGE;
+
 	/* TIMx_ARR register shouldn't be buffered (ARPE=0) */
 	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
 	regmap_write(priv->regmap, TIM_ARR, ceiling);
@@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
 	priv->regmap = ddata->regmap;
 	priv->clk = ddata->clk;
 	priv->ceiling = ddata->max_arr;
+	priv->max_arr = ddata->max_arr;
 
 	priv->counter.name = dev_name(dev);
 	priv->counter.parent = dev;
-- 
2.7.4


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 2028 bytes --]

On Tue, Mar 02, 2021 at 03:43:55PM +0100, Fabrice Gasnier wrote:
> The ceiling value isn't checked before writing it into registers. The user
> could write a value higher than the counter resolution (e.g. 16 or 32 bits
> indicated by max_arr). This makes most significant bits to be truncated.
> Fix it by checking the max_arr to report a range error [1] to the user.
> 
> Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")
> 
> [1] https://lkml.org/lkml/2021/2/12/358
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

Side question: if priv->ceiling is tracking the current ceiling
configuration, would it make sense to change stm32_count_ceiling_read()
to print the value of priv->ceiling instead of doing a regmap_read()
call?

> ---
>  drivers/counter/stm32-timer-cnt.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> index ef2a974..2cf0c05 100644
> --- a/drivers/counter/stm32-timer-cnt.c
> +++ b/drivers/counter/stm32-timer-cnt.c
> @@ -32,6 +32,7 @@ struct stm32_timer_cnt {
>  	struct regmap *regmap;
>  	struct clk *clk;
>  	u32 ceiling;
> +	u32 max_arr;
>  	bool enabled;
>  	struct stm32_timer_regs bak;
>  };
> @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter,
>  	if (ret)
>  		return ret;
>  
> +	if (ceiling > priv->max_arr)
> +		return -ERANGE;
> +
>  	/* TIMx_ARR register shouldn't be buffered (ARPE=0) */
>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>  	regmap_write(priv->regmap, TIM_ARR, ceiling);
> @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>  	priv->regmap = ddata->regmap;
>  	priv->clk = ddata->clk;
>  	priv->ceiling = ddata->max_arr;
> +	priv->max_arr = ddata->max_arr;
>  
>  	priv->counter.name = dev_name(dev);
>  	priv->counter.parent = dev;
> -- 
> 2.7.4
> 

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[Fabrice Gasnier]

On 3/2/21 3:56 PM, William Breathitt Gray wrote:
> On Tue, Mar 02, 2021 at 03:43:55PM +0100, Fabrice Gasnier wrote:
>> The ceiling value isn't checked before writing it into registers. The user
>> could write a value higher than the counter resolution (e.g. 16 or 32 bits
>> indicated by max_arr). This makes most significant bits to be truncated.
>> Fix it by checking the max_arr to report a range error [1] to the user.
>>
>> Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")
>>
>> [1] https://lkml.org/lkml/2021/2/12/358
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>
> 
> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> 
> Side question: if priv->ceiling is tracking the current ceiling
> configuration, would it make sense to change stm32_count_ceiling_read()
> to print the value of priv->ceiling instead of doing a regmap_read()
> call?

Hi William,

Thanks for reviewing.

I'd be fine either way. So no objection to move to the priv->ceiling
(cached) value. It could also here here.
By looking at this, I figured out there's probably another thing to fix
here, for initial conditions.

At probe time priv->ceiling is initialized to max value (ex 65535 for a
16 bits counter). But the register content is 0 (clear by mfd driver at
probe time).

- So, reading ceiling from sysfs currently reports 0 (regmap_read())
after booting and probing.

I see two cases at this point:
- In case the counter gets enabled without any prior configuration, it
won't count: ceiling value (e.g. 65535) should be written to register
before it is enabled, so the counter will actually count. So there's
room for a fix here.

- In case function gets set (ex: quadrature x4), priv->ceiling (e.g.
65535) gets written to the register (although it's been read earlier as
0 from sysfs).
This could be fixed by reading the priv->ceiling in
stm32_count_ceiling_read() as you're asking (provided 1st case has been
fixed as well)

I'll probably prepare one or two patches for the above cases, if you agree ?

Best Regards,
Fabrice

> 
>> ---
>>  drivers/counter/stm32-timer-cnt.c | 5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
>> index ef2a974..2cf0c05 100644
>> --- a/drivers/counter/stm32-timer-cnt.c
>> +++ b/drivers/counter/stm32-timer-cnt.c
>> @@ -32,6 +32,7 @@ struct stm32_timer_cnt {
>>  	struct regmap *regmap;
>>  	struct clk *clk;
>>  	u32 ceiling;
>> +	u32 max_arr;
>>  	bool enabled;
>>  	struct stm32_timer_regs bak;
>>  };
>> @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter,
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (ceiling > priv->max_arr)
>> +		return -ERANGE;
>> +
>>  	/* TIMx_ARR register shouldn't be buffered (ARPE=0) */
>>  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
>>  	regmap_write(priv->regmap, TIM_ARR, ceiling);
>> @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
>>  	priv->regmap = ddata->regmap;
>>  	priv->clk = ddata->clk;
>>  	priv->ceiling = ddata->max_arr;
>> +	priv->max_arr = ddata->max_arr;
>>  
>>  	priv->counter.name = dev_name(dev);
>>  	priv->counter.parent = dev;
>> -- 
>> 2.7.4
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[William Breathitt Gray]

[-- Attachment #1.1: Type: text/plain, Size: 2320 bytes --]

On Tue, Mar 02, 2021 at 06:03:25PM +0100, Fabrice Gasnier wrote:
> On 3/2/21 3:56 PM, William Breathitt Gray wrote:
> > Side question: if priv->ceiling is tracking the current ceiling
> > configuration, would it make sense to change stm32_count_ceiling_read()
> > to print the value of priv->ceiling instead of doing a regmap_read()
> > call?
> 
> Hi William,
> 
> Thanks for reviewing.
> 
> I'd be fine either way. So no objection to move to the priv->ceiling
> (cached) value. It could also here here.
> By looking at this, I figured out there's probably another thing to fix
> here, for initial conditions.
> 
> At probe time priv->ceiling is initialized to max value (ex 65535 for a
> 16 bits counter). But the register content is 0 (clear by mfd driver at
> probe time).
> 
> - So, reading ceiling from sysfs currently reports 0 (regmap_read())
> after booting and probing.
> 
> I see two cases at this point:
> - In case the counter gets enabled without any prior configuration, it
> won't count: ceiling value (e.g. 65535) should be written to register
> before it is enabled, so the counter will actually count. So there's
> room for a fix here.
> 
> - In case function gets set (ex: quadrature x4), priv->ceiling (e.g.
> 65535) gets written to the register (although it's been read earlier as
> 0 from sysfs).
> This could be fixed by reading the priv->ceiling in
> stm32_count_ceiling_read() as you're asking (provided 1st case has been
> fixed as well)
> 
> I'll probably prepare one or two patches for the above cases, if you agree ?
> 
> Best Regards,
> Fabrice

Looking through the driver, it doesn't seem like priv->ceiling is used
in any performance critical code, just the callbacks for count_write()
and function_set(). It might make more sense to remove priv->ceiling and
replace it with the regmap_read() calls where necessary so that we
always get the most current ceiling value from the device when needed.

As for the default ceiling value for the device at probe time, this
should probably be set to the max value because that is what a normal
user would expect when loading a Counter driver (a ceiling value of 0 at
startup is somewhat unintuitive).

If you prepare those two patches, then that should resolve this.

William Breathitt Gray

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[Fabrice Gasnier]

On 3/3/21 12:42 AM, William Breathitt Gray wrote:
> On Tue, Mar 02, 2021 at 06:03:25PM +0100, Fabrice Gasnier wrote:
>> On 3/2/21 3:56 PM, William Breathitt Gray wrote:
>>> Side question: if priv->ceiling is tracking the current ceiling
>>> configuration, would it make sense to change stm32_count_ceiling_read()
>>> to print the value of priv->ceiling instead of doing a regmap_read()
>>> call?
>>
>> Hi William,
>>
>> Thanks for reviewing.
>>
>> I'd be fine either way. So no objection to move to the priv->ceiling
>> (cached) value. It could also here here.
>> By looking at this, I figured out there's probably another thing to fix
>> here, for initial conditions.
>>
>> At probe time priv->ceiling is initialized to max value (ex 65535 for a
>> 16 bits counter). But the register content is 0 (clear by mfd driver at
>> probe time).
>>
>> - So, reading ceiling from sysfs currently reports 0 (regmap_read())
>> after booting and probing.
>>
>> I see two cases at this point:
>> - In case the counter gets enabled without any prior configuration, it
>> won't count: ceiling value (e.g. 65535) should be written to register
>> before it is enabled, so the counter will actually count. So there's
>> room for a fix here.
>>
>> - In case function gets set (ex: quadrature x4), priv->ceiling (e.g.
>> 65535) gets written to the register (although it's been read earlier as
>> 0 from sysfs).
>> This could be fixed by reading the priv->ceiling in
>> stm32_count_ceiling_read() as you're asking (provided 1st case has been
>> fixed as well)
>>
>> I'll probably prepare one or two patches for the above cases, if you agree ?
>>
>> Best Regards,
>> Fabrice
> 
> Looking through the driver, it doesn't seem like priv->ceiling is used
> in any performance critical code, just the callbacks for count_write()
> and function_set(). It might make more sense to remove priv->ceiling and
> replace it with the regmap_read() calls where necessary so that we
> always get the most current ceiling value from the device when needed.

Hi William,

Ok, I'll look into this.

> 
> As for the default ceiling value for the device at probe time, this
> should probably be set to the max value because that is what a normal
> user would expect when loading a Counter driver (a ceiling value of 0 at
> startup is somewhat unintuitive).

Yes, I agree. In fact, the default (reset) value is the maximum. The 0
value is forced in ARR register by the mfd driver [1], after reading the
max_arr value. I see no rationale for this.
I think the fix should probably be done there: I'd prefer to backup and
restore ARR value in the mfd, instead of unconditionally clearing it.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/drivers/mfd/stm32-timers.c?h=v5.11#n167

> 
> If you prepare those two patches, then that should resolve this.

I'll prepare this.

Thanks for your advices,
Best Regards,
Fabrice

> 
> William Breathitt Gray
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] counter: stm32-timer-cnt: fix ceiling write max value

[Jonathan Cameron]

On Tue, 2 Mar 2021 23:56:46 +0900
William Breathitt Gray <vilhelm.gray@gmail.com> wrote:

> On Tue, Mar 02, 2021 at 03:43:55PM +0100, Fabrice Gasnier wrote:
> > The ceiling value isn't checked before writing it into registers. The user
> > could write a value higher than the counter resolution (e.g. 16 or 32 bits
> > indicated by max_arr). This makes most significant bits to be truncated.
> > Fix it by checking the max_arr to report a range error [1] to the user.
> > 
> > Fixes: ad29937e206f ("counter: Add STM32 Timer quadrature encoder")
> > 
> > [1] https://lkml.org/lkml/2021/2/12/358
> > 
> > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com>  
> 
> Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>
Applied to the fixes-togreg branch of iio.git and marked for stable.

thanks

Jonathan
> 
> Side question: if priv->ceiling is tracking the current ceiling
> configuration, would it make sense to change stm32_count_ceiling_read()
> to print the value of priv->ceiling instead of doing a regmap_read()
> call?
> 
> > ---
> >  drivers/counter/stm32-timer-cnt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/counter/stm32-timer-cnt.c b/drivers/counter/stm32-timer-cnt.c
> > index ef2a974..2cf0c05 100644
> > --- a/drivers/counter/stm32-timer-cnt.c
> > +++ b/drivers/counter/stm32-timer-cnt.c
> > @@ -32,6 +32,7 @@ struct stm32_timer_cnt {
> >  	struct regmap *regmap;
> >  	struct clk *clk;
> >  	u32 ceiling;
> > +	u32 max_arr;
> >  	bool enabled;
> >  	struct stm32_timer_regs bak;
> >  };
> > @@ -185,6 +186,9 @@ static ssize_t stm32_count_ceiling_write(struct counter_device *counter,
> >  	if (ret)
> >  		return ret;
> >  
> > +	if (ceiling > priv->max_arr)
> > +		return -ERANGE;
> > +
> >  	/* TIMx_ARR register shouldn't be buffered (ARPE=0) */
> >  	regmap_update_bits(priv->regmap, TIM_CR1, TIM_CR1_ARPE, 0);
> >  	regmap_write(priv->regmap, TIM_ARR, ceiling);
> > @@ -360,6 +364,7 @@ static int stm32_timer_cnt_probe(struct platform_device *pdev)
> >  	priv->regmap = ddata->regmap;
> >  	priv->clk = ddata->clk;
> >  	priv->ceiling = ddata->max_arr;
> > +	priv->max_arr = ddata->max_arr;
> >  
> >  	priv->counter.name = dev_name(dev);
> >  	priv->counter.parent = dev;
> > -- 
> > 2.7.4
> >   


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel