| Message ID | a05454f8-e409-4f60-93f7-6aa2ea0a2a23@stanley.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ep93xx: clock: Fix off by one in ep93xx_div_recalc_rate() | expand |
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]);
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]);
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
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)
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!
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]);
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(-)