| Message ID | 20200224125202.13784-1-amadeuszx.slawinski@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8308a09e87d2cb51adb186dc7d5a5c1913fb0758 |
| Headers | show |
| Series | ASoC: Intel: Skylake: Fix available clock counter incrementation | expand |
On 2020-02-24 13:52, Amadeusz Sławiński wrote: > Incrementation of avail_clk_cnt was incorrectly moved to error path. Put > it back to success path. > > Fixes: 6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev') > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/intel/skylake/skl-ssp-clk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c > index 1c0e5226cb5b..bd43885f3805 100644 > --- a/sound/soc/intel/skylake/skl-ssp-clk.c > +++ b/sound/soc/intel/skylake/skl-ssp-clk.c > @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct platform_device *pdev) > &clks[i], clk_pdata, i); > > if (IS_ERR(data->clk[data->avail_clk_cnt])) { > - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); > + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); > goto err_unreg_skl_clk; > } > + > + data->avail_clk_cnt++; > } > > platform_set_drvdata(pdev, data); > Thank you for the fix. Reviewed-by: Cezary Rojewski <cezary.rojewski@intel.com>
On 2/24/20 6:52 AM, Amadeusz Sławiński wrote: > Incrementation of avail_clk_cnt was incorrectly moved to error path. Put > it back to success path. > > Fixes: 6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev') > Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> > --- > sound/soc/intel/skylake/skl-ssp-clk.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c > index 1c0e5226cb5b..bd43885f3805 100644 > --- a/sound/soc/intel/skylake/skl-ssp-clk.c > +++ b/sound/soc/intel/skylake/skl-ssp-clk.c > @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct platform_device *pdev) > &clks[i], clk_pdata, i); > > if (IS_ERR(data->clk[data->avail_clk_cnt])) { > - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); > + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); Are you sure? If you start with avail_clk_cnt set to zero, the error handling will decrement and access offset -1 static void unregister_src_clk(struct skl_clk_data *dclk) { while (dclk->avail_clk_cnt--) clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup); <<< oob access with offset -1 } > goto err_unreg_skl_clk; > } > + > + data->avail_clk_cnt++; > } > > platform_set_drvdata(pdev, data); >
On 2/24/2020 5:18 PM, Pierre-Louis Bossart wrote: > > > On 2/24/20 6:52 AM, Amadeusz Sławiński wrote: >> Incrementation of avail_clk_cnt was incorrectly moved to error path. Put >> it back to success path. >> >> Fixes: 6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference >> when unloading clk dev') >> Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> >> --- >> sound/soc/intel/skylake/skl-ssp-clk.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c >> b/sound/soc/intel/skylake/skl-ssp-clk.c >> index 1c0e5226cb5b..bd43885f3805 100644 >> --- a/sound/soc/intel/skylake/skl-ssp-clk.c >> +++ b/sound/soc/intel/skylake/skl-ssp-clk.c >> @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct >> platform_device *pdev) >> &clks[i], clk_pdata, i); >> if (IS_ERR(data->clk[data->avail_clk_cnt])) { >> - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); >> + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); > > Are you sure? > > If you start with avail_clk_cnt set to zero, the error handling will > decrement and access offset -1 > Yes, I'm sure as far as I know c it will first check the value and then decrement it, so it will be 0 while doing the "while" check and it won't enter the loop. You can double check with simplified usecase: #include <stdio.h> int main() { int i = 0; while(i--) printf("do something with i, while i = %d\n", i); } which seems to work fine to me, by not entering the loop. Use case is as following: we start with avail_clk_cnt = 0; register clock at index 0; increment avail_clk_cnt to 1; register clock at index 1; increment avail_clk_cnt to 2; register clock at index 2; increment avail_clk_cnt to 3 now let's assume that there is no more clocks to register so we do our stuff and then we need to free clocks so we enter loop 3 evaluates to true, so we decrement it and release clock at index 2 2 evaluates to true, so we decrement it and release clock at index 1 1 evaluates to true, so we decrement it and release clock at index 0 0 evaluates to false, so wo don't enter loop similar thing happens if we fail to register clock and do error handling Amadeusz
On 2020-02-24 17:18, Pierre-Louis Bossart wrote: > On 2/24/20 6:52 AM, Amadeusz Sławiński wrote: >> Incrementation of avail_clk_cnt was incorrectly moved to error path. Put >> it back to success path. >> >> diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c >> b/sound/soc/intel/skylake/skl-ssp-clk.c >> index 1c0e5226cb5b..bd43885f3805 100644 >> --- a/sound/soc/intel/skylake/skl-ssp-clk.c >> +++ b/sound/soc/intel/skylake/skl-ssp-clk.c >> @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct >> platform_device *pdev) >> &clks[i], clk_pdata, i); >> if (IS_ERR(data->clk[data->avail_clk_cnt])) { >> - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); >> + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); > > Are you sure? > > If you start with avail_clk_cnt set to zero, the error handling will > decrement and access offset -1 > > static void unregister_src_clk(struct skl_clk_data *dclk) > { > while (dclk->avail_clk_cnt--) > clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup); <<< oob > access with offset -1 > } > Decrementation will occur after while's check evaluation - loop will exit, decrementation happens regardless.
On 2/24/20 11:42 AM, Cezary Rojewski wrote: > On 2020-02-24 17:18, Pierre-Louis Bossart wrote: >> On 2/24/20 6:52 AM, Amadeusz Sławiński wrote: >>> Incrementation of avail_clk_cnt was incorrectly moved to error path. Put >>> it back to success path. >>> > >>> diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c >>> b/sound/soc/intel/skylake/skl-ssp-clk.c >>> index 1c0e5226cb5b..bd43885f3805 100644 >>> --- a/sound/soc/intel/skylake/skl-ssp-clk.c >>> +++ b/sound/soc/intel/skylake/skl-ssp-clk.c >>> @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct >>> platform_device *pdev) >>> &clks[i], clk_pdata, i); >>> if (IS_ERR(data->clk[data->avail_clk_cnt])) { >>> - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); >>> + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); >> >> Are you sure? >> >> If you start with avail_clk_cnt set to zero, the error handling will >> decrement and access offset -1 >> >> static void unregister_src_clk(struct skl_clk_data *dclk) >> { >> while (dclk->avail_clk_cnt--) >> clkdev_drop(dclk->clk[dclk->avail_clk_cnt]->lookup); <<< oob >> access with offset -1 >> } >> > > Decrementation will occur after while's check evaluation - loop will > exit, decrementation happens regardless. ok, I need more coffee I guess :-) Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
diff --git a/sound/soc/intel/skylake/skl-ssp-clk.c b/sound/soc/intel/skylake/skl-ssp-clk.c index 1c0e5226cb5b..bd43885f3805 100644 --- a/sound/soc/intel/skylake/skl-ssp-clk.c +++ b/sound/soc/intel/skylake/skl-ssp-clk.c @@ -384,9 +384,11 @@ static int skl_clk_dev_probe(struct platform_device *pdev) &clks[i], clk_pdata, i); if (IS_ERR(data->clk[data->avail_clk_cnt])) { - ret = PTR_ERR(data->clk[data->avail_clk_cnt++]); + ret = PTR_ERR(data->clk[data->avail_clk_cnt]); goto err_unreg_skl_clk; } + + data->avail_clk_cnt++; } platform_set_drvdata(pdev, data);
Incrementation of avail_clk_cnt was incorrectly moved to error path. Put it back to success path. Fixes: 6ee927f2f01466 ('ASoC: Intel: Skylake: Fix NULL ptr dereference when unloading clk dev') Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com> --- sound/soc/intel/skylake/skl-ssp-clk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)