| Message ID | 20220617133330.6510-1-matthias.bgg@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | PM / devfreq: mediatek: Fix possible dereference of null pointer | expand |
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; >
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; >> > >
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;