opp: fix use after free in _update_opp_table_clk()

[PATCH] opp: fix use after free in _update_opp_table_clk()

Posted by Peter Griffin 2 weeks, 3 days ago

dev_pm_opp_put_opp_table() frees the opp_table which is subsquently used
by dev_err_probe(). This causes an Oops during boot on gs101-oriole.

cpu cpu0: error 000000006b6b6b6b: Couldn't find clock
Unable to handle kernel paging request at virtual address 006b6b6b6b6b6cd3
...
Hardware name: Oriole (DT)
pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
pc : _of_add_table_indexed+0x80/0xbb0
lr : _of_add_table_indexed+0x6c/0xbb0
...
Call trace:
 _of_add_table_indexed+0x80/0xbb0 (P)
 dev_pm_opp_of_cpumask_add_table+0x70/0x120
 dt_cpufreq_probe+0x23c/0x480
 platform_probe+0x64/0xb8

Fixes: 84f05af0975c9 ("opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer")
Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
---
This UAF regression was introduced in v7.3-rc1, and causes an Oops on
boot for gs101-oriole (in part due to our default upstream kernel dev
config having memory poisoning enabled, hence the 0x6b6b6b6b
pattern). It is intended that this patch should get picked for the
next -rc.

Peter
---
---
 drivers/opp/core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2fafd983de8f5..c8b941cb81b44 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1581,6 +1581,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 					       struct opp_table *opp_table,
 					       bool getclk)
 {
+	int ret;
 	/*
 	 * Return early if we don't need to get clk or we have already done it
 	 * earlier.
@@ -1607,9 +1608,10 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
 	opp_table->clk = clk_get_optional(dev, NULL);
 
 	if (IS_ERR(opp_table->clk)) {
-		dev_pm_opp_put_opp_table(opp_table);
 		dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");
-		return ERR_CAST(opp_table->clk);
+		ret = PTR_ERR(opp_table->clk);
+		dev_pm_opp_put_opp_table(opp_table);
+		return ERR_PTR(ret);
 	}
 
 	if (opp_table->clk)

---
base-commit: df2908090cda368b01ff43709f51890076c56157
change-id: 20260908-opp-core-uaf-update-opp-table-0d9ea3741231

Best regards,
-- 
Peter Griffin <peter.griffin@linaro.org>

Re: [PATCH] opp: fix use after free in _update_opp_table_clk()

Posted by Tudor Ambarus 2 weeks, 3 days ago


On 9/8/26 3:37 PM, Peter Griffin wrote:
> dev_pm_opp_put_opp_table() frees the opp_table which is subsquently used
> by dev_err_probe(). This causes an Oops during boot on gs101-oriole.
> 
> cpu cpu0: error 000000006b6b6b6b: Couldn't find clock
> Unable to handle kernel paging request at virtual address 006b6b6b6b6b6cd3
> ...
> Hardware name: Oriole (DT)
> pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> pc : _of_add_table_indexed+0x80/0xbb0
> lr : _of_add_table_indexed+0x6c/0xbb0
> ...
> Call trace:
>  _of_add_table_indexed+0x80/0xbb0 (P)
>  dev_pm_opp_of_cpumask_add_table+0x70/0x120
>  dt_cpufreq_probe+0x23c/0x480
>  platform_probe+0x64/0xb8
> 
> Fixes: 84f05af0975c9 ("opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer")
> Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> ---
> This UAF regression was introduced in v7.3-rc1, and causes an Oops on
> boot for gs101-oriole (in part due to our default upstream kernel dev
> config having memory poisoning enabled, hence the 0x6b6b6b6b
> pattern). It is intended that this patch should get picked for the
> next -rc.
> 
> Peter
> ---
> ---
>  drivers/opp/core.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 2fafd983de8f5..c8b941cb81b44 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1581,6 +1581,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>  					       struct opp_table *opp_table,
>  					       bool getclk)
>  {
> +	int ret;
>  	/*
>  	 * Return early if we don't need to get clk or we have already done it
>  	 * earlier.
> @@ -1607,9 +1608,10 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
>  	opp_table->clk = clk_get_optional(dev, NULL);
>  
>  	if (IS_ERR(opp_table->clk)) {
> -		dev_pm_opp_put_opp_table(opp_table);
>  		dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");

you can do ret = dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");
> -		return ERR_CAST(opp_table->clk);
> +		ret = PTR_ERR(opp_table->clk);

and you won't need this duplicated line.

with this addressed:
Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

> +		dev_pm_opp_put_opp_table(opp_table);
> +		return ERR_PTR(ret);
>  	}
>  
>  	if (opp_table->clk)
> 
> ---
> base-commit: df2908090cda368b01ff43709f51890076c56157
> change-id: 20260908-opp-core-uaf-update-opp-table-0d9ea3741231
> 
> Best regards,

Re: [PATCH] opp: fix use after free in _update_opp_table_clk()

Posted by Viresh Kumar 2 weeks, 2 days ago

On 08-09-26, 16:41, Tudor Ambarus wrote:
> 
> 
> On 9/8/26 3:37 PM, Peter Griffin wrote:
> > dev_pm_opp_put_opp_table() frees the opp_table which is subsquently used
> > by dev_err_probe(). This causes an Oops during boot on gs101-oriole.
> > 
> > cpu cpu0: error 000000006b6b6b6b: Couldn't find clock
> > Unable to handle kernel paging request at virtual address 006b6b6b6b6b6cd3
> > ...
> > Hardware name: Oriole (DT)
> > pstate: 00400005 (nzcv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > pc : _of_add_table_indexed+0x80/0xbb0
> > lr : _of_add_table_indexed+0x6c/0xbb0
> > ...
> > Call trace:
> >  _of_add_table_indexed+0x80/0xbb0 (P)
> >  dev_pm_opp_of_cpumask_add_table+0x70/0x120
> >  dt_cpufreq_probe+0x23c/0x480
> >  platform_probe+0x64/0xb8
> > 
> > Fixes: 84f05af0975c9 ("opp: Use clk_get_optional() to avoid leaving opp_table->clk as an error pointer")
> > Signed-off-by: Peter Griffin <peter.griffin@linaro.org>
> > ---
> > This UAF regression was introduced in v7.3-rc1, and causes an Oops on
> > boot for gs101-oriole (in part due to our default upstream kernel dev
> > config having memory poisoning enabled, hence the 0x6b6b6b6b
> > pattern). It is intended that this patch should get picked for the
> > next -rc.
> > 
> > Peter
> > ---
> > ---
> >  drivers/opp/core.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> > index 2fafd983de8f5..c8b941cb81b44 100644
> > --- a/drivers/opp/core.c
> > +++ b/drivers/opp/core.c
> > @@ -1581,6 +1581,7 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> >  					       struct opp_table *opp_table,
> >  					       bool getclk)
> >  {
> > +	int ret;
> >  	/*
> >  	 * Return early if we don't need to get clk or we have already done it
> >  	 * earlier.
> > @@ -1607,9 +1608,10 @@ static struct opp_table *_update_opp_table_clk(struct device *dev,
> >  	opp_table->clk = clk_get_optional(dev, NULL);
> >  
> >  	if (IS_ERR(opp_table->clk)) {
> > -		dev_pm_opp_put_opp_table(opp_table);
> >  		dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");
> 
> you can do ret = dev_err_probe(dev, PTR_ERR(opp_table->clk), "Couldn't find clock\n");
> > -		return ERR_CAST(opp_table->clk);
> > +		ret = PTR_ERR(opp_table->clk);
> 
> and you won't need this duplicated line.
> 
> with this addressed:
> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org>

Applied with the fix. Thanks.

-- 
viresh