[PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[Liu Xiang]

When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
success. Even a group of pins call sunxi_pmx_request(), the refcount
is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
To solve this problem, go to err path if regulator_get() return NULL
or error.

Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
---
 drivers/pinctrl/sunxi/pinctrl-sunxi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.c b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
index dc8d39ae0..d1a8974eb 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.c
@@ -777,7 +777,7 @@ static int sunxi_pmx_request(struct pinctrl_dev *pctldev, unsigned offset)
 
 	snprintf(supply, sizeof(supply), "vcc-p%c", 'a' + bank);
 	reg = regulator_get(pctl->dev, supply);
-	if (IS_ERR(reg)) {
+	if (IS_ERR_OR_NULL(reg)) {
 		dev_err(pctl->dev, "Couldn't get bank P%c regulator\n",
 			'A' + bank);
 		return PTR_ERR(reg);
@@ -811,7 +811,7 @@ static int sunxi_pmx_free(struct pinctrl_dev *pctldev, unsigned offset)
 					    PINS_PER_BANK;
 	struct sunxi_pinctrl_regulator *s_reg = &pctl->regulators[bank_offset];
 
-	if (!refcount_dec_and_test(&s_reg->refcount))
+	if (!s_reg->regulator || !refcount_dec_and_test(&s_reg->refcount))
 		return 0;
 
 	regulator_disable(s_reg->regulator);
-- 
2.17.1

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 600 bytes --]

Hi,

On Tue, Jan 19, 2021 at 02:29:08PM +0800, Liu Xiang wrote:
> When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
> success. Even a group of pins call sunxi_pmx_request(), the refcount
> is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
> To solve this problem, go to err path if regulator_get() return NULL
> or error.
> 
> Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>

Is there any drawback to depending on CONFIG_REGULATOR?

Given that we need those regulators enabled anyway, I guess we could
just select or depends on it

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[liu xiang]

> Hi,

> On Tue, Jan 19, 2021 at 02:29:08PM +0800, Liu Xiang wrote:
> When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
> success. Even a group of pins call sunxi_pmx_request(), the refcount
> is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
> To solve this problem, go to err path if regulator_get() return NULL
> or error.
> 
> Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>

> Is there any drawback to depending on CONFIG_REGULATOR?

> Given that we need those regulators enabled anyway, I guess we could
> just select or depends on it
>
> Maxime


Yes, I think so. But CONFIG_REGULATOR is not enabled by default now.
So I can find this problem during startup.

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[Linus Walleij]

On Thu, Jan 21, 2021 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Jan 19, 2021 at 02:29:08PM +0800, Liu Xiang wrote:
> > When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
> > success. Even a group of pins call sunxi_pmx_request(), the refcount
> > is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
> > To solve this problem, go to err path if regulator_get() return NULL
> > or error.
> >
> > Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
>
> Is there any drawback to depending on CONFIG_REGULATOR?
>
> Given that we need those regulators enabled anyway, I guess we could
> just select or depends on it

I agree.

Liu can you make a patch to Kconfig to just select REGULATOR?
Possibly even the specific regulator driver this SoC is using
if it is very specific for this purpose.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[liu xiang]

------------------------------------------------------------------

> On Thu, Jan 21, 2021 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Jan 19, 2021 at 02:29:08PM +0800, Liu Xiang wrote:
> > When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
> > success. Even a group of pins call sunxi_pmx_request(), the refcount
> > is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
> > To solve this problem, go to err path if regulator_get() return NULL
> > or error.
> >
> > Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
>
> Is there any drawback to depending on CONFIG_REGULATOR?
>
> Given that we need those regulators enabled anyway, I guess we could
> just select or depends on it
>
> I agree.
>
> Liu can you make a patch to Kconfig to just select REGULATOR?
> Possibly even the specific regulator driver this SoC is using
> if it is very specific for this purpose.
>
> Yours,
> Linus Walleij

Sure. I will send a new patch.

Yours,
Liu Xiang

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[liu xiang]

> On Thu, Jan 21, 2021 at 5:40 PM Maxime Ripard <maxime@cerno.tech> wrote:
> On Tue, Jan 19, 2021 at 02:29:08PM +0800, Liu Xiang wrote:
> > When CONFIG_REGULATOR is not set, sunxi_pmx_request() always return
> > success. Even a group of pins call sunxi_pmx_request(), the refcount
> > is only 1. This can cause a use-after-free warning in sunxi_pmx_free().
> > To solve this problem, go to err path if regulator_get() return NULL
> > or error.
> >
> > Signed-off-by: Liu Xiang <liu.xiang@zlingsmart.com>
>
> Is there any drawback to depending on CONFIG_REGULATOR?
>
> Given that we need those regulators enabled anyway, I guess we could
> just select or depends on it
> 
> I agree.
> 
> Liu can you make a patch to Kconfig to just select REGULATOR?
> Possibly even the specific regulator driver this SoC is using
> if it is very specific for this purpose.
> 
> Yours,
> Linus Walleij

I found that the regulator driver is related to the specific board, not the SoC.
There is no board config for ARM64 SoC like ARM.
Is a good idea to select the regulator driver in the pinctrl Konfig? Or just 
select CONFIG_REGULATOR_FIXED_VOLTAGE to avoid the use-after-free warning?

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[Linus Walleij]

On Tue, Jan 26, 2021 at 7:31 AM liu xiang <liu.xiang@zlingsmart.com> wrote:

> > Liu can you make a patch to Kconfig to just select REGULATOR?
> > Possibly even the specific regulator driver this SoC is using
> > if it is very specific for this purpose.
>
> I found that the regulator driver is related to the specific board, not the SoC.
> There is no board config for ARM64 SoC like ARM.
> Is a good idea to select the regulator driver in the pinctrl Konfig? Or just
> select CONFIG_REGULATOR_FIXED_VOLTAGE to avoid the use-after-free warning?

If that regulator is what the board uses to satisfy this driver then that
is what you should select. Write some blurb in the commit message
about what is going on.

You can even add a comment in Kconfig like that:

# Needed to provide power to rails
select REGULATOR_FIXED_VOLTAGE

Yours,
Linus Walleij

Re: [PATCH] pinctrl: sunxi: fix use-after-free in sunxi_pmx_free()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1273 bytes --]

On Tue, Jan 26, 2021 at 04:03:29PM +0100, Linus Walleij wrote:
> On Tue, Jan 26, 2021 at 7:31 AM liu xiang <liu.xiang@zlingsmart.com> wrote:
> 
> > > Liu can you make a patch to Kconfig to just select REGULATOR?
> > > Possibly even the specific regulator driver this SoC is using
> > > if it is very specific for this purpose.
> >
> > I found that the regulator driver is related to the specific board, not the SoC.
> > There is no board config for ARM64 SoC like ARM.
> > Is a good idea to select the regulator driver in the pinctrl Konfig? Or just
> > select CONFIG_REGULATOR_FIXED_VOLTAGE to avoid the use-after-free warning?
> 
> If that regulator is what the board uses to satisfy this driver then that
> is what you should select. Write some blurb in the commit message
> about what is going on.
> 
> You can even add a comment in Kconfig like that:
> 
> # Needed to provide power to rails
> select REGULATOR_FIXED_VOLTAGE

Virtually all the boards will need a regulator, but you can't make the
assumption that this is the driver that is going to be used. In most
case, it isn't.

But it's not really a big deal, we depend on the framework itself being
enabled for regulator_get to return the proper error, not one given
driver.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]