ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

[PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Dan Carpenter 2 months, 2 weeks ago

The psc->div[] array has psc->num_div elements.  These values come from
when we call clk_hw_register_div().  It's adc_divisors and
ARRAY_SIZE(adc_divisors)) and so on.  So this condition needs to be >=
instead of > to prevent an out of bounds read.

Fixes: 9645ccc7bd7a ("ep93xx: clock: convert in-place to COMMON_CLK")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 arch/arm/mach-ep93xx/clock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
index 85a496ddc619..e9f72a529b50 100644
--- a/arch/arm/mach-ep93xx/clock.c
+++ b/arch/arm/mach-ep93xx/clock.c
@@ -359,7 +359,7 @@ static unsigned long ep93xx_div_recalc_rate(struct clk_hw *hw,
 	u32 val = __raw_readl(psc->reg);
 	u8 index = (val & psc->mask) >> psc->shift;
 
-	if (index > psc->num_div)
+	if (index >= psc->num_div)
 		return 0;
 
 	return DIV_ROUND_UP_ULL(parent_rate, psc->div[index]);
-- 
2.45.2

Re: [PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Nikita Shubin 2 months, 2 weeks ago

Hi Dan!

Reviewed-by: Nikita Shubin <nikita.shubin@maquefel.me>

Alexander, Arnd

unfortunately, the ep93xx DT conversion series is also affected by this
bug.

On Wed, 2024-09-11 at 10:39 +0300, Dan Carpenter wrote:
> The psc->div[] array has psc->num_div elements.  These values come
> from
> when we call clk_hw_register_div().  It's adc_divisors and
> ARRAY_SIZE(adc_divisors)) and so on.  So this condition needs to be
> >=
> instead of > to prevent an out of bounds read.
> 
> Fixes: 9645ccc7bd7a ("ep93xx: clock: convert in-place to COMMON_CLK")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  arch/arm/mach-ep93xx/clock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-
> ep93xx/clock.c
> index 85a496ddc619..e9f72a529b50 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -359,7 +359,7 @@ static unsigned long
> ep93xx_div_recalc_rate(struct clk_hw *hw,
>         u32 val = __raw_readl(psc->reg);
>         u8 index = (val & psc->mask) >> psc->shift;
>  
> -       if (index > psc->num_div)
> +       if (index >= psc->num_div)
>                 return 0;
>  
>         return DIV_ROUND_UP_ULL(parent_rate, psc->div[index]);

Re: [PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Arnd Bergmann 2 months, 2 weeks ago

On Wed, Sep 11, 2024, at 08:14, Nikita Shubin wrote:
> Hi Dan!
>
> Reviewed-by: Nikita Shubin <nikita.shubin@maquefel.me>
>
> Alexander, Arnd
>
> unfortunately, the ep93xx DT conversion series is also affected by this
> bug.

Here is what I did now:

1. applied Dan's patch on a new branch
2. applied the DT conversion series on top of that,
   removing that file.
3. applied the first patch (with minor context changes)
   in drivers/clk/clk-ep93xx.c again, along with
   the MODULE_LICENSE fix I did.
4. finally, merged the entire branch into my for-next
   branch so it actually makes it into linux-next

My plan now is to keep the branch in linux-next for at
least a week and send all the other pull requests for
the merge window first. If no other problems show up
(either with this branch or my other 6.12 contents),
I hope to send it all later in the merge window. If
something goes wrong, I'll send only the bugfix as part
of my first fixes branch for 6.12 and we'll defer the
DT conversion once more.

I should have merged it earlier, but wasn't sure about
interdependencies with the parts that already got merged
elsewhere and with the comments about DTC warnings.

From what I can tell, the current state is as good as
it gets, as we'll always get more comments or conflicts
with new reversions of the series. Let's hope we can
address any other issues on top of what I've merged
now and stop rebasing.

    Arnd

Re: [PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Alexander Sverdlin 2 months, 2 weeks ago

Hi Arnd, Nikita,

On Wed, 2024-09-11 at 14:54 +0000, Arnd Bergmann wrote:
> On Wed, Sep 11, 2024, at 08:14, Nikita Shubin wrote:
> > Hi Dan!
> > 
> > Reviewed-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > 
> > Alexander, Arnd
> > 
> > unfortunately, the ep93xx DT conversion series is also affected by this
> > bug.
> 
> Here is what I did now:
> 
> 1. applied Dan's patch on a new branch
> 2. applied the DT conversion series on top of that,
>    removing that file.
> 3. applied the first patch (with minor context changes)
>    in drivers/clk/clk-ep93xx.c again, along with
>    the MODULE_LICENSE fix I did.
> 4. finally, merged the entire branch into my for-next
>    branch so it actually makes it into linux-next
> 
> My plan now is to keep the branch in linux-next for at
> least a week and send all the other pull requests for
> the merge window first. If no other problems show up
> (either with this branch or my other 6.12 contents),
> I hope to send it all later in the merge window. If
> something goes wrong, I'll send only the bugfix as part
> of my first fixes branch for 6.12 and we'll defer the
> DT conversion once more.
> 
> I should have merged it earlier, but wasn't sure about
> interdependencies with the parts that already got merged
> elsewhere and with the comments about DTC warnings.
> 
> From what I can tell, the current state is as good as
> it gets, as we'll always get more comments or conflicts
> with new reversions of the series. Let's hope we can
> address any other issues on top of what I've merged
> now and stop rebasing.

thanks Arnd for resolving this finally and Nikita for
your relentless efforts!

PS. I've archived Subject patch now in soc patchwork
(because I think I've messed up the author info, but
all of this seems to be obsolete now)

-- 
Alexander Sverdlin.

Re: [PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Nikita Shubin 2 months, 2 weeks ago

Hello Alexander, Arnd,

On Wed, 2024-09-11 at 16:59 +0200, Alexander Sverdlin wrote:
> Hi Arnd, Nikita,
> 
> On Wed, 2024-09-11 at 14:54 +0000, Arnd Bergmann wrote:
> > On Wed, Sep 11, 2024, at 08:14, Nikita Shubin wrote:
> > > Hi Dan!
> > > 
> > > Reviewed-by: Nikita Shubin <nikita.shubin@maquefel.me>
> > > 
> > > Alexander, Arnd
> > > 
> > > unfortunately, the ep93xx DT conversion series is also affected
> > > by this
> > > bug.
> > 
> > Here is what I did now:
> > 
> > 1. applied Dan's patch on a new branch
> > 2. applied the DT conversion series on top of that,
> >    removing that file.
> > 3. applied the first patch (with minor context changes)
> >    in drivers/clk/clk-ep93xx.c again, along with
> >    the MODULE_LICENSE fix I did.
> > 4. finally, merged the entire branch into my for-next
> >    branch so it actually makes it into linux-next
> > 
> > My plan now is to keep the branch in linux-next for at
> > least a week and send all the other pull requests for
> > the merge window first. If no other problems show up
> > (either with this branch or my other 6.12 contents),
> > I hope to send it all later in the merge window. If
> > something goes wrong, I'll send only the bugfix as part
> > of my first fixes branch for 6.12 and we'll defer the
> > DT conversion once more.
> > 
> > I should have merged it earlier, but wasn't sure about
> > interdependencies with the parts that already got merged
> > elsewhere and with the comments about DTC warnings.
> > 
> > From what I can tell, the current state is as good as
> > it gets, as we'll always get more comments or conflicts
> > with new reversions of the series. Let's hope we can
> > address any other issues on top of what I've merged
> > now and stop rebasing.
> 
> thanks Arnd for resolving this finally and Nikita for
> your relentless efforts!
> 
> PS. I've archived Subject patch now in soc patchwork
> (because I think I've messed up the author info, but
> all of this seems to be obsolete now)
> 

thank you for your work and support!

Re: [PATCH] ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate()

Posted by Alexander Sverdlin 2 months, 2 weeks ago

Hi Dan!

On Wed, 2024-09-11 at 10:39 +0300, Dan Carpenter wrote:
> The psc->div[] array has psc->num_div elements.  These values come from
> when we call clk_hw_register_div().  It's adc_divisors and
> ARRAY_SIZE(adc_divisors)) and so on.  So this condition needs to be >=
> instead of > to prevent an out of bounds read.
> 
> Fixes: 9645ccc7bd7a ("ep93xx: clock: convert in-place to COMMON_CLK")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>

Acked-by: Alexander Sverdlin <alexander.sverdlin@gmail.com>

> ---
>  arch/arm/mach-ep93xx/clock.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/mach-ep93xx/clock.c b/arch/arm/mach-ep93xx/clock.c
> index 85a496ddc619..e9f72a529b50 100644
> --- a/arch/arm/mach-ep93xx/clock.c
> +++ b/arch/arm/mach-ep93xx/clock.c
> @@ -359,7 +359,7 @@ static unsigned long ep93xx_div_recalc_rate(struct clk_hw *hw,
>  	u32 val = __raw_readl(psc->reg);
>  	u8 index = (val & psc->mask) >> psc->shift;
>  
> -	if (index > psc->num_div)
> +	if (index >= psc->num_div)
>  		return 0;
>  
>  	return DIV_ROUND_UP_ULL(parent_rate, psc->div[index]);

-- 
Alexander Sverdlin.