[PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation
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);
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
The patch
ASoC: Intel: Skylake: Fix available clock counter incrementation
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 8308a09e87d2cb51adb186dc7d5a5c1913fb0758 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Mon, 24 Feb 2020 07:52:02 -0500 Subject: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
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 Reviewed-by: Cezary Rojewski cezary.rojewski@intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200224125202.13784-1-amadeuszx.slawinski@linux.i... Signed-off-by: Mark Brown broonie@kernel.org --- 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);
participants (4)
-
Amadeusz Sławiński -
Cezary Rojewski -
Mark Brown -
Pierre-Louis Bossart