| Message ID | 20200819161945.1155298-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | pinctrl: mediatek: Free eint data on failure | expand |
On Wed, Aug 19, 2020 at 9:19 AM Enric Balletbo i Serra <enric.balletbo@collabora.com> wrote: > > The pinctrl driver can work without the EINT resource, but, if it is > expected to have this resource but the mtk_build_eint() function fails > after allocating their data (because can't get the resource or can't map > the irq), the data is not freed and you end with a NULL pointer > dereference. Fix this by freeing the data if mtk_build_eint() fails, so > pinctrl still works and doesn't hang. > > This is noticeable after commit f97dbf48ca43 ("irqchip/mtk-sysirq: Convert > to a platform driver") on MT8183 because, due this commit, the pinctrl driver > fails to map the irq and spots the following bug: > > [ 1.947597] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004 > [ 1.956404] Mem abort info: > [ 1.959203] ESR = 0x96000004 > [ 1.962259] EC = 0x25: DABT (current EL), IL = 32 bits > [ 1.967565] SET = 0, FnV = 0 > [ 1.970613] EA = 0, S1PTW = 0 > [ 1.973747] Data abort info: > [ 1.976619] ISV = 0, ISS = 0x00000004 > [ 1.980447] CM = 0, WnR = 0 > [ 1.983410] [0000000000000004] user address but active_mm is swapper > [ 1.989759] Internal error: Oops: 96000004 [#1] PREEMPT SMP > [ 1.995322] Modules linked in: > [ 1.998371] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc1+ #44 > [ 2.004715] Hardware name: MediaTek krane sku176 board (DT) > [ 2.010280] pstate: 60000005 (nZCv daif -PAN -UAO BTYPE=--) > [ 2.015850] pc : mtk_eint_set_debounce+0x48/0x1b8 > [ 2.020546] lr : mtk_eint_set_debounce+0x34/0x1b8 > [ 2.025239] sp : ffff80001008baa0 > [ 2.028544] x29: ffff80001008baa0 x28: ffff0000ff7ff790 > [ 2.033847] x27: ffff0000f9ec34b0 x26: ffff0000f9ec3480 > [ 2.039150] x25: ffff0000fa576410 x24: ffff0000fa502800 > [ 2.044453] x23: 0000000000001388 x22: ffff0000fa635f80 > [ 2.049755] x21: 0000000000000008 x20: 0000000000000000 > [ 2.055058] x19: 0000000000000071 x18: 0000000000000001 > [ 2.060360] x17: 0000000000000000 x16: 0000000000000000 > [ 2.065662] x15: ffff0000facc8470 x14: ffffffffffffffff > [ 2.070965] x13: 0000000000000001 x12: 00000000000000c0 > [ 2.076267] x11: 0000000000000040 x10: 0000000000000070 > [ 2.081569] x9 : ffffaec0063d24d8 x8 : ffff0000fa800270 > [ 2.086872] x7 : 0000000000000000 x6 : 0000000000000011 > [ 2.092174] x5 : ffff0000fa800248 x4 : ffff0000fa800270 > [ 2.097476] x3 : ffff8000100c5000 x2 : 0000000000000000 > [ 2.102778] x1 : 0000000000000000 x0 : 0000000000000000 > [ 2.108081] Call trace: > [ 2.110520] mtk_eint_set_debounce+0x48/0x1b8 > [ 2.114870] mtk_gpio_set_config+0x5c/0x78 > [ 2.118958] gpiod_set_config+0x5c/0x78 > [ 2.122786] gpiod_set_debounce+0x18/0x28 > [ 2.126789] gpio_keys_probe+0x50c/0x910 > [ 2.130705] platform_drv_probe+0x54/0xa8 > [ 2.134705] really_probe+0xe4/0x3b0 > [ 2.138271] driver_probe_device+0x58/0xb8 > [ 2.142358] device_driver_attach+0x74/0x80 > [ 2.146532] __driver_attach+0x58/0xe0 > [ 2.150274] bus_for_each_dev+0x70/0xc0 > [ 2.154100] driver_attach+0x24/0x30 > [ 2.157666] bus_add_driver+0x14c/0x1f0 > [ 2.161493] driver_register+0x64/0x120 > [ 2.165319] __platform_driver_register+0x48/0x58 > [ 2.170017] gpio_keys_init+0x1c/0x28 > [ 2.173672] do_one_initcall+0x54/0x1b4 > [ 2.177499] kernel_init_freeable+0x1d0/0x238 > [ 2.181848] kernel_init+0x14/0x118 > [ 2.185328] ret_from_fork+0x10/0x34 > [ 2.188899] Code: a9438ac1 12001266 f94006c3 121e766a (b9400421) > [ 2.194991] ---[ end trace 168cf7b3324b6570 ]--- > [ 2.199611] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b > [ 2.207260] SMP: stopping secondary CPUs > [ 2.211294] Kernel Offset: 0x2ebff4800000 from 0xffff800010000000 > [ 2.217377] PHYS_OFFSET: 0xffffb50500000000 > [ 2.221551] CPU features: 0x0240002,2188200c > [ 2.225811] Memory Limit: none > [ 2.228860] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]--- > > Fixes: 89132dd8ffd2 ("pinctrl: mediatek: extend eint build to pinctrl-mtk-common-v2.c") > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> > --- > > .../pinctrl/mediatek/pinctrl-mtk-common-v2.c | 27 ++++++++++++++----- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > index 2f3dfb56c3fa..4b532e6b9038 100644 > --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c > @@ -355,6 +355,7 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > { > struct device_node *np = pdev->dev.of_node; > struct resource *res; > + int ret; > > if (!IS_ENABLED(CONFIG_EINT_MTK)) > return 0; > @@ -369,19 +370,26 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); > if (!res) { > dev_err(&pdev->dev, "Unable to get eint resource\n"); > - return -ENODEV; > + ret = -ENODEV; > + goto err_free_eint; > } > > hw->eint->base = devm_ioremap_resource(&pdev->dev, res); > - if (IS_ERR(hw->eint->base)) > - return PTR_ERR(hw->eint->base); > + if (IS_ERR(hw->eint->base)) { > + ret = PTR_ERR(hw->eint->base); > + goto err_free_eint; > + } > > hw->eint->irq = irq_of_parse_and_map(np, 0); > - if (!hw->eint->irq) > - return -EINVAL; > + if (!hw->eint->irq) { > + ret = -EINVAL; > + goto err_free_eint; > + } > > - if (!hw->soc->eint_hw) > - return -ENODEV; > + if (!hw->soc->eint_hw) { > + ret = -ENODEV; > + goto err_free_eint; > + } > > hw->eint->dev = &pdev->dev; > hw->eint->hw = hw->soc->eint_hw; > @@ -389,6 +397,11 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) > hw->eint->gpio_xlate = &mtk_eint_xt; > > return mtk_eint_do_init(hw->eint); > + Similarly, I think we have to call devm_ioremap_release to explicitly free resource when a certain failure occurs after devm_ioremap_resource, right? > +err_free_eint: > + devm_kfree(hw->dev, hw->eint); > + hw->eint = NULL; > + return ret; > } > EXPORT_SYMBOL_GPL(mtk_build_eint); > > -- > 2.28.0 >
Hi Sean, On 20/8/20 2:00, Sean Wang wrote: > On Wed, Aug 19, 2020 at 9:19 AM Enric Balletbo i Serra > <enric.balletbo@collabora.com> wrote: >> >> The pinctrl driver can work without the EINT resource, but, if it is >> expected to have this resource but the mtk_build_eint() function fails >> after allocating their data (because can't get the resource or can't map >> the irq), the data is not freed and you end with a NULL pointer >> dereference. Fix this by freeing the data if mtk_build_eint() fails, so >> pinctrl still works and doesn't hang. >> >> This is noticeable after commit f97dbf48ca43 ("irqchip/mtk-sysirq: Convert >> to a platform driver") on MT8183 because, due this commit, the pinctrl driver >> fails to map the irq and spots the following bug: >> >> [ 1.947597] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004 >> [ 1.956404] Mem abort info: >> [ 1.959203] ESR = 0x96000004 >> [ 1.962259] EC = 0x25: DABT (current EL), IL = 32 bits >> [ 1.967565] SET = 0, FnV = 0 >> [ 1.970613] EA = 0, S1PTW = 0 >> [ 1.973747] Data abort info: >> [ 1.976619] ISV = 0, ISS = 0x00000004 >> [ 1.980447] CM = 0, WnR = 0 >> [ 1.983410] [0000000000000004] user address but active_mm is swapper >> [ 1.989759] Internal error: Oops: 96000004 [#1] PREEMPT SMP >> [ 1.995322] Modules linked in: >> [ 1.998371] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc1+ #44 >> [ 2.004715] Hardware name: MediaTek krane sku176 board (DT) >> [ 2.010280] pstate: 60000005 (nZCv daif -PAN -UAO BTYPE=--) >> [ 2.015850] pc : mtk_eint_set_debounce+0x48/0x1b8 >> [ 2.020546] lr : mtk_eint_set_debounce+0x34/0x1b8 >> [ 2.025239] sp : ffff80001008baa0 >> [ 2.028544] x29: ffff80001008baa0 x28: ffff0000ff7ff790 >> [ 2.033847] x27: ffff0000f9ec34b0 x26: ffff0000f9ec3480 >> [ 2.039150] x25: ffff0000fa576410 x24: ffff0000fa502800 >> [ 2.044453] x23: 0000000000001388 x22: ffff0000fa635f80 >> [ 2.049755] x21: 0000000000000008 x20: 0000000000000000 >> [ 2.055058] x19: 0000000000000071 x18: 0000000000000001 >> [ 2.060360] x17: 0000000000000000 x16: 0000000000000000 >> [ 2.065662] x15: ffff0000facc8470 x14: ffffffffffffffff >> [ 2.070965] x13: 0000000000000001 x12: 00000000000000c0 >> [ 2.076267] x11: 0000000000000040 x10: 0000000000000070 >> [ 2.081569] x9 : ffffaec0063d24d8 x8 : ffff0000fa800270 >> [ 2.086872] x7 : 0000000000000000 x6 : 0000000000000011 >> [ 2.092174] x5 : ffff0000fa800248 x4 : ffff0000fa800270 >> [ 2.097476] x3 : ffff8000100c5000 x2 : 0000000000000000 >> [ 2.102778] x1 : 0000000000000000 x0 : 0000000000000000 >> [ 2.108081] Call trace: >> [ 2.110520] mtk_eint_set_debounce+0x48/0x1b8 >> [ 2.114870] mtk_gpio_set_config+0x5c/0x78 >> [ 2.118958] gpiod_set_config+0x5c/0x78 >> [ 2.122786] gpiod_set_debounce+0x18/0x28 >> [ 2.126789] gpio_keys_probe+0x50c/0x910 >> [ 2.130705] platform_drv_probe+0x54/0xa8 >> [ 2.134705] really_probe+0xe4/0x3b0 >> [ 2.138271] driver_probe_device+0x58/0xb8 >> [ 2.142358] device_driver_attach+0x74/0x80 >> [ 2.146532] __driver_attach+0x58/0xe0 >> [ 2.150274] bus_for_each_dev+0x70/0xc0 >> [ 2.154100] driver_attach+0x24/0x30 >> [ 2.157666] bus_add_driver+0x14c/0x1f0 >> [ 2.161493] driver_register+0x64/0x120 >> [ 2.165319] __platform_driver_register+0x48/0x58 >> [ 2.170017] gpio_keys_init+0x1c/0x28 >> [ 2.173672] do_one_initcall+0x54/0x1b4 >> [ 2.177499] kernel_init_freeable+0x1d0/0x238 >> [ 2.181848] kernel_init+0x14/0x118 >> [ 2.185328] ret_from_fork+0x10/0x34 >> [ 2.188899] Code: a9438ac1 12001266 f94006c3 121e766a (b9400421) >> [ 2.194991] ---[ end trace 168cf7b3324b6570 ]--- >> [ 2.199611] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b >> [ 2.207260] SMP: stopping secondary CPUs >> [ 2.211294] Kernel Offset: 0x2ebff4800000 from 0xffff800010000000 >> [ 2.217377] PHYS_OFFSET: 0xffffb50500000000 >> [ 2.221551] CPU features: 0x0240002,2188200c >> [ 2.225811] Memory Limit: none >> [ 2.228860] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]--- >> >> Fixes: 89132dd8ffd2 ("pinctrl: mediatek: extend eint build to pinctrl-mtk-common-v2.c") >> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> >> --- >> >> .../pinctrl/mediatek/pinctrl-mtk-common-v2.c | 27 ++++++++++++++----- >> 1 file changed, 20 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c >> index 2f3dfb56c3fa..4b532e6b9038 100644 >> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c >> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c >> @@ -355,6 +355,7 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) >> { >> struct device_node *np = pdev->dev.of_node; >> struct resource *res; >> + int ret; >> >> if (!IS_ENABLED(CONFIG_EINT_MTK)) >> return 0; >> @@ -369,19 +370,26 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) >> res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); >> if (!res) { >> dev_err(&pdev->dev, "Unable to get eint resource\n"); >> - return -ENODEV; >> + ret = -ENODEV; >> + goto err_free_eint; >> } >> >> hw->eint->base = devm_ioremap_resource(&pdev->dev, res); >> - if (IS_ERR(hw->eint->base)) >> - return PTR_ERR(hw->eint->base); >> + if (IS_ERR(hw->eint->base)) { >> + ret = PTR_ERR(hw->eint->base); >> + goto err_free_eint; >> + } >> >> hw->eint->irq = irq_of_parse_and_map(np, 0); >> - if (!hw->eint->irq) >> - return -EINVAL; >> + if (!hw->eint->irq) { >> + ret = -EINVAL; >> + goto err_free_eint; >> + } >> >> - if (!hw->soc->eint_hw) >> - return -ENODEV; >> + if (!hw->soc->eint_hw) { >> + ret = -ENODEV; >> + goto err_free_eint; >> + } >> >> hw->eint->dev = &pdev->dev; >> hw->eint->hw = hw->soc->eint_hw; >> @@ -389,6 +397,11 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) >> hw->eint->gpio_xlate = &mtk_eint_xt; >> >> return mtk_eint_do_init(hw->eint); >> + > > Similarly, I think we have to call devm_ioremap_release to explicitly > free resource when a certain failure occurs after > devm_ioremap_resource, right? > Right, I'll do this for the second version. Thanks, Enric >> +err_free_eint: >> + devm_kfree(hw->dev, hw->eint); >> + hw->eint = NULL; >> + return ret; >> } >> EXPORT_SYMBOL_GPL(mtk_build_eint); >> >> -- >> 2.28.0 >>
diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c index 2f3dfb56c3fa..4b532e6b9038 100644 --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c @@ -355,6 +355,7 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; struct resource *res; + int ret; if (!IS_ENABLED(CONFIG_EINT_MTK)) return 0; @@ -369,19 +370,26 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "eint"); if (!res) { dev_err(&pdev->dev, "Unable to get eint resource\n"); - return -ENODEV; + ret = -ENODEV; + goto err_free_eint; } hw->eint->base = devm_ioremap_resource(&pdev->dev, res); - if (IS_ERR(hw->eint->base)) - return PTR_ERR(hw->eint->base); + if (IS_ERR(hw->eint->base)) { + ret = PTR_ERR(hw->eint->base); + goto err_free_eint; + } hw->eint->irq = irq_of_parse_and_map(np, 0); - if (!hw->eint->irq) - return -EINVAL; + if (!hw->eint->irq) { + ret = -EINVAL; + goto err_free_eint; + } - if (!hw->soc->eint_hw) - return -ENODEV; + if (!hw->soc->eint_hw) { + ret = -ENODEV; + goto err_free_eint; + } hw->eint->dev = &pdev->dev; hw->eint->hw = hw->soc->eint_hw; @@ -389,6 +397,11 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct platform_device *pdev) hw->eint->gpio_xlate = &mtk_eint_xt; return mtk_eint_do_init(hw->eint); + +err_free_eint: + devm_kfree(hw->dev, hw->eint); + hw->eint = NULL; + return ret; } EXPORT_SYMBOL_GPL(mtk_build_eint);
The pinctrl driver can work without the EINT resource, but, if it is expected to have this resource but the mtk_build_eint() function fails after allocating their data (because can't get the resource or can't map the irq), the data is not freed and you end with a NULL pointer dereference. Fix this by freeing the data if mtk_build_eint() fails, so pinctrl still works and doesn't hang. This is noticeable after commit f97dbf48ca43 ("irqchip/mtk-sysirq: Convert to a platform driver") on MT8183 because, due this commit, the pinctrl driver fails to map the irq and spots the following bug: [ 1.947597] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000004 [ 1.956404] Mem abort info: [ 1.959203] ESR = 0x96000004 [ 1.962259] EC = 0x25: DABT (current EL), IL = 32 bits [ 1.967565] SET = 0, FnV = 0 [ 1.970613] EA = 0, S1PTW = 0 [ 1.973747] Data abort info: [ 1.976619] ISV = 0, ISS = 0x00000004 [ 1.980447] CM = 0, WnR = 0 [ 1.983410] [0000000000000004] user address but active_mm is swapper [ 1.989759] Internal error: Oops: 96000004 [#1] PREEMPT SMP [ 1.995322] Modules linked in: [ 1.998371] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc1+ #44 [ 2.004715] Hardware name: MediaTek krane sku176 board (DT) [ 2.010280] pstate: 60000005 (nZCv daif -PAN -UAO BTYPE=--) [ 2.015850] pc : mtk_eint_set_debounce+0x48/0x1b8 [ 2.020546] lr : mtk_eint_set_debounce+0x34/0x1b8 [ 2.025239] sp : ffff80001008baa0 [ 2.028544] x29: ffff80001008baa0 x28: ffff0000ff7ff790 [ 2.033847] x27: ffff0000f9ec34b0 x26: ffff0000f9ec3480 [ 2.039150] x25: ffff0000fa576410 x24: ffff0000fa502800 [ 2.044453] x23: 0000000000001388 x22: ffff0000fa635f80 [ 2.049755] x21: 0000000000000008 x20: 0000000000000000 [ 2.055058] x19: 0000000000000071 x18: 0000000000000001 [ 2.060360] x17: 0000000000000000 x16: 0000000000000000 [ 2.065662] x15: ffff0000facc8470 x14: ffffffffffffffff [ 2.070965] x13: 0000000000000001 x12: 00000000000000c0 [ 2.076267] x11: 0000000000000040 x10: 0000000000000070 [ 2.081569] x9 : ffffaec0063d24d8 x8 : ffff0000fa800270 [ 2.086872] x7 : 0000000000000000 x6 : 0000000000000011 [ 2.092174] x5 : ffff0000fa800248 x4 : ffff0000fa800270 [ 2.097476] x3 : ffff8000100c5000 x2 : 0000000000000000 [ 2.102778] x1 : 0000000000000000 x0 : 0000000000000000 [ 2.108081] Call trace: [ 2.110520] mtk_eint_set_debounce+0x48/0x1b8 [ 2.114870] mtk_gpio_set_config+0x5c/0x78 [ 2.118958] gpiod_set_config+0x5c/0x78 [ 2.122786] gpiod_set_debounce+0x18/0x28 [ 2.126789] gpio_keys_probe+0x50c/0x910 [ 2.130705] platform_drv_probe+0x54/0xa8 [ 2.134705] really_probe+0xe4/0x3b0 [ 2.138271] driver_probe_device+0x58/0xb8 [ 2.142358] device_driver_attach+0x74/0x80 [ 2.146532] __driver_attach+0x58/0xe0 [ 2.150274] bus_for_each_dev+0x70/0xc0 [ 2.154100] driver_attach+0x24/0x30 [ 2.157666] bus_add_driver+0x14c/0x1f0 [ 2.161493] driver_register+0x64/0x120 [ 2.165319] __platform_driver_register+0x48/0x58 [ 2.170017] gpio_keys_init+0x1c/0x28 [ 2.173672] do_one_initcall+0x54/0x1b4 [ 2.177499] kernel_init_freeable+0x1d0/0x238 [ 2.181848] kernel_init+0x14/0x118 [ 2.185328] ret_from_fork+0x10/0x34 [ 2.188899] Code: a9438ac1 12001266 f94006c3 121e766a (b9400421) [ 2.194991] ---[ end trace 168cf7b3324b6570 ]--- [ 2.199611] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b [ 2.207260] SMP: stopping secondary CPUs [ 2.211294] Kernel Offset: 0x2ebff4800000 from 0xffff800010000000 [ 2.217377] PHYS_OFFSET: 0xffffb50500000000 [ 2.221551] CPU features: 0x0240002,2188200c [ 2.225811] Memory Limit: none [ 2.228860] ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b ]--- Fixes: 89132dd8ffd2 ("pinctrl: mediatek: extend eint build to pinctrl-mtk-common-v2.c") Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com> --- .../pinctrl/mediatek/pinctrl-mtk-common-v2.c | 27 ++++++++++++++----- 1 file changed, 20 insertions(+), 7 deletions(-)