Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Cezary Rojewski]

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>

[PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Amadeusz Sławiński]

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);
-- 
2.17.1

Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Pierre-Louis Bossart]

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);
> 

Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Amadeusz Sławiński]

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

Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Cezary Rojewski]

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.

Re: [PATCH] ASoC: Intel: Skylake: Fix available clock counter incrementation

[Pierre-Louis Bossart]

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>

Applied "ASoC: Intel: Skylake: Fix available clock counter incrementation" to the asoc tree

[Mark Brown]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 2563 bytes --]

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.intel.com
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);
-- 
2.20.1