[PATCH] PM / devfreq: mediatek: Fix possible dereference of null pointer

[PATCH] PM / devfreq: mediatek: Fix possible dereference of null pointer

[matthias.bgg]

From: Matthias Brugger <matthias.bgg@gmail.com>

We dereference the driver data before checking of it's valid.
This patch fixes this, by accessing the PLL data struct after cheching
the pointer

Fixes: 07dc787be231 ("Add linux-next specific files for 20220617")
Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
---
 drivers/devfreq/mtk-cci-devfreq.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
index 71abb3fbd042..77522f16c878 100644
--- a/drivers/devfreq/mtk-cci-devfreq.c
+++ b/drivers/devfreq/mtk-cci-devfreq.c
@@ -127,7 +127,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
 			      u32 flags)
 {
 	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
-	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
+	struct clk *cci_pll;
 	struct dev_pm_opp *opp;
 	unsigned long opp_rate;
 	int voltage, pre_voltage, inter_voltage, target_voltage, ret;
@@ -138,6 +138,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
 	if (drv->pre_freq == *freq)
 		return 0;
 
+	cci_pll = clk_get_parent(drv->cci_clk);
 	inter_voltage = drv->inter_voltage;
 
 	opp_rate = *freq;
-- 
2.36.0

Re: [PATCH] PM / devfreq: mediatek: Fix possible dereference of null pointer

[AngeloGioacchino Del Regno]

Il 17/06/22 15:33, matthias.bgg@kernel.org ha scritto:
> From: Matthias Brugger <matthias.bgg@gmail.com>
> 
> We dereference the driver data before checking of it's valid.
> This patch fixes this, by accessing the PLL data struct after cheching
> the pointer
> 

Hello Matthias,

honestly, I don't think that this commit is right: mtk_ccifreq_target() is the
devfreq_dev_profile's .target() callback!

Checking mtk_ccifreq_probe(), we are setting drvdata long before adding the
devfreq device so, actually, it's impossible for dev_get_drvdata(dev) to return
NULL, or whatever invalid pointer.

This means that the right thing to do in mtk_ccifreq_target() is to simply remove
the `drv` NULL check, as this can never happen! :-)

Cheers,
Angelo

> Fixes: 07dc787be231 ("Add linux-next specific files for 20220617")
> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
> ---
>   drivers/devfreq/mtk-cci-devfreq.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/devfreq/mtk-cci-devfreq.c b/drivers/devfreq/mtk-cci-devfreq.c
> index 71abb3fbd042..77522f16c878 100644
> --- a/drivers/devfreq/mtk-cci-devfreq.c
> +++ b/drivers/devfreq/mtk-cci-devfreq.c
> @@ -127,7 +127,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
>   			      u32 flags)
>   {
>   	struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
> -	struct clk *cci_pll = clk_get_parent(drv->cci_clk);
> +	struct clk *cci_pll;
>   	struct dev_pm_opp *opp;
>   	unsigned long opp_rate;
>   	int voltage, pre_voltage, inter_voltage, target_voltage, ret;
> @@ -138,6 +138,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned long *freq,
>   	if (drv->pre_freq == *freq)
>   		return 0;
>   
> +	cci_pll = clk_get_parent(drv->cci_clk);
>   	inter_voltage = drv->inter_voltage;
>   
>   	opp_rate = *freq;
> 

Re: [PATCH] PM / devfreq: mediatek: Fix possible dereference of null pointer

[Matthias Brugger]

On 20/06/2022 16:21, AngeloGioacchino Del Regno wrote:
> Il 17/06/22 15:33, matthias.bgg@kernel.org ha scritto:
>> From: Matthias Brugger <matthias.bgg@gmail.com>
>>
>> We dereference the driver data before checking of it's valid.
>> This patch fixes this, by accessing the PLL data struct after cheching
>> the pointer
>>
> 
> Hello Matthias,
> 
> honestly, I don't think that this commit is right: mtk_ccifreq_target() is the
> devfreq_dev_profile's .target() callback!
> 
> Checking mtk_ccifreq_probe(), we are setting drvdata long before adding the
> devfreq device so, actually, it's impossible for dev_get_drvdata(dev) to return
> NULL, or whatever invalid pointer.
> 
> This means that the right thing to do in mtk_ccifreq_target() is to simply remove
> the `drv` NULL check, as this can never happen! :-)
> 

Yes you are right. Probe will error out if drv can't be allocated or cci_clk 
isn't present. I'll send a new patch.

Thanks,
Matthias

> Cheers,
> Angelo
> 
>> Fixes: 07dc787be231 ("Add linux-next specific files for 20220617")
>> Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
>> ---
>>   drivers/devfreq/mtk-cci-devfreq.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/devfreq/mtk-cci-devfreq.c 
>> b/drivers/devfreq/mtk-cci-devfreq.c
>> index 71abb3fbd042..77522f16c878 100644
>> --- a/drivers/devfreq/mtk-cci-devfreq.c
>> +++ b/drivers/devfreq/mtk-cci-devfreq.c
>> @@ -127,7 +127,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned 
>> long *freq,
>>                     u32 flags)
>>   {
>>       struct mtk_ccifreq_drv *drv = dev_get_drvdata(dev);
>> -    struct clk *cci_pll = clk_get_parent(drv->cci_clk);
>> +    struct clk *cci_pll;
>>       struct dev_pm_opp *opp;
>>       unsigned long opp_rate;
>>       int voltage, pre_voltage, inter_voltage, target_voltage, ret;
>> @@ -138,6 +138,7 @@ static int mtk_ccifreq_target(struct device *dev, unsigned 
>> long *freq,
>>       if (drv->pre_freq == *freq)
>>           return 0;
>> +    cci_pll = clk_get_parent(drv->cci_clk);
>>       inter_voltage = drv->inter_voltage;
>>       opp_rate = *freq;
>>
> 
>