| Message ID | 20210119062908.20169-1-liu.xiang@zlingsmart.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: sunxi: fix use-after-free in sunxi_pmx_free() | expand |
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
> 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.
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
------------------------------------------------------------------ > 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
> 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?
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
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
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);
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(-)