Group index is incremented on every new group parsed. Since the
field is part of struct imx_pinctrl_soc_info, which is typically
a global variable passed by the individual pinctrl-imx.c based
driver, it does not get cleared automatically when re-probing the
driver. This lead imx_pinctrl_parse_functions passing a group
pointer which is outside of the allocated group space on second
probe and onwards. Typically this ended up in a NULL pointer
dereference when accessing the name field like this:
Unable to handle kernel NULL pointer dereference at virtual address 00000000
...
PC is at strcmp+0x18/0x44
LR is at imx_dt_node_to_map+0xc4/0x290
Avoid this by setting group_index to 0 on probe.
This has been observed when using DEBUG_TEST_DRIVER_REMOVE.
Signed-off-by: Stefan Agner <[email protected]>
---
drivers/pinctrl/freescale/pinctrl-imx.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
index 4761320..79c4e14 100644
--- a/drivers/pinctrl/freescale/pinctrl-imx.c
+++ b/drivers/pinctrl/freescale/pinctrl-imx.c
@@ -687,6 +687,7 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
if (!info->functions)
return -ENOMEM;
+ info->group_index = 0;
if (flat_funcs) {
info->ngroups = of_get_child_count(np);
} else {
--
2.10.0
Hi Stefan,
On 19.10.2016 00:09, Stefan Agner wrote:
> Group index is incremented on every new group parsed. Since the
> field is part of struct imx_pinctrl_soc_info, which is typically
> a global variable passed by the individual pinctrl-imx.c based
> driver, it does not get cleared automatically when re-probing the
> driver. This lead imx_pinctrl_parse_functions passing a group
> pointer which is outside of the allocated group space on second
> probe and onwards. Typically this ended up in a NULL pointer
> dereference when accessing the name field like this:
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> PC is at strcmp+0x18/0x44
> LR is at imx_dt_node_to_map+0xc4/0x290
>
> Avoid this by setting group_index to 0 on probe.
>
> This has been observed when using DEBUG_TEST_DRIVER_REMOVE.
>
> Signed-off-by: Stefan Agner <[email protected]>
I've recently developed a pinctrl driver for the last remaining
i.MX31 platform from the SoC series (not published yet), it does
not fit well under any of the 3 existing drivers, and instead
of adding 1.5K lines of code with half of them almost copy-pasted
from the rest drivers I started to work on generalization of the
i.MX/Vybrid pinctrl/pinmux drivers.
This task is large and I'm not sure when I complete and share
the results, but your change conflicts with one of the changes
in my backlog... Let me share it with you right now, because it
deals with info->group_index from another point of view and it
(unlikely) may have an impact on Vybrid platform, which I can not
test.
> ---
> drivers/pinctrl/freescale/pinctrl-imx.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/pinctrl/freescale/pinctrl-imx.c b/drivers/pinctrl/freescale/pinctrl-imx.c
> index 4761320..79c4e14 100644
> --- a/drivers/pinctrl/freescale/pinctrl-imx.c
> +++ b/drivers/pinctrl/freescale/pinctrl-imx.c
> @@ -687,6 +687,7 @@ static int imx_pinctrl_probe_dt(struct platform_device *pdev,
> if (!info->functions)
> return -ENOMEM;
>
> + info->group_index = 0;
> if (flat_funcs) {
> info->ngroups = of_get_child_count(np);
> } else {
>
--
With best wishes,
Vladimir
On Tue, Oct 18, 2016 at 11:09 PM, Stefan Agner <[email protected]> wrote:
> Group index is incremented on every new group parsed. Since the
> field is part of struct imx_pinctrl_soc_info, which is typically
> a global variable passed by the individual pinctrl-imx.c based
> driver, it does not get cleared automatically when re-probing the
> driver. This lead imx_pinctrl_parse_functions passing a group
> pointer which is outside of the allocated group space on second
> probe and onwards. Typically this ended up in a NULL pointer
> dereference when accessing the name field like this:
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> ...
> PC is at strcmp+0x18/0x44
> LR is at imx_dt_node_to_map+0xc4/0x290
>
> Avoid this by setting group_index to 0 on probe.
>
> This has been observed when using DEBUG_TEST_DRIVER_REMOVE.
>
> Signed-off-by: Stefan Agner <[email protected]>
Patch applied for fixes.
Should it be tagged for stable too?
Yours,
Linus Walleij
On Wed, Oct 19, 2016 at 12:40 AM, Vladimir Zapolskiy <[email protected]> wrote:
> I've recently developed a pinctrl driver for the last remaining
> i.MX31 platform from the SoC series (not published yet), it does
> not fit well under any of the 3 existing drivers, and instead
> of adding 1.5K lines of code with half of them almost copy-pasted
> from the rest drivers I started to work on generalization of the
> i.MX/Vybrid pinctrl/pinmux drivers.
That is a noble task.
> This task is large and I'm not sure when I complete and share
> the results, but your change conflicts with one of the changes
> in my backlog... Let me share it with you right now, because it
> deals with info->group_index from another point of view and it
> (unlikely) may have an impact on Vybrid platform, which I can not
> test.
I have to merge this patch to fix an upstream regression
(I guess? No response from mantainers) so I guee you'll have
to rebase your series onto whatever release candidate has it.
Shouldn't be too hard I guess.
Yours,
Linus Walleij
Hi Linus, Stefan,
On 25.10.2016 12:11, Linus Walleij wrote:
> On Wed, Oct 19, 2016 at 12:40 AM, Vladimir Zapolskiy <[email protected]> wrote:
>
>> I've recently developed a pinctrl driver for the last remaining
>> i.MX31 platform from the SoC series (not published yet), it does
>> not fit well under any of the 3 existing drivers, and instead
>> of adding 1.5K lines of code with half of them almost copy-pasted
>> from the rest drivers I started to work on generalization of the
>> i.MX/Vybrid pinctrl/pinmux drivers.
>
> That is a noble task.
>
>> This task is large and I'm not sure when I complete and share
>> the results, but your change conflicts with one of the changes
>> in my backlog... Let me share it with you right now, because it
>> deals with info->group_index from another point of view and it
>> (unlikely) may have an impact on Vybrid platform, which I can not
>> test.
>
> I have to merge this patch to fix an upstream regression
> (I guess? No response from mantainers) so I guee you'll have
> to rebase your series onto whatever release candidate has it.
>
> Shouldn't be too hard I guess.
>
correct, thank you for merging Stefan's fix.
--
With best wishes,
Vladimir