[PATCH] pinctrl: imx: reset group index on probe

[PATCH] pinctrl: imx: reset group index on probe

[Stefan Agner]

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 <stefan@agner.ch>
---
 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

Re: [PATCH] pinctrl: imx: reset group index on probe

[Vladimir Zapolskiy]

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 <stefan@agner.ch>

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

Re: [PATCH] pinctrl: imx: reset group index on probe

[Linus Walleij]

On Tue, Oct 18, 2016 at 11:09 PM, Stefan Agner <stefan@agner.ch> 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 <stefan@agner.ch>

Patch applied for fixes.

Should it be tagged for stable too?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: imx: reset group index on probe

[Linus Walleij]

On Wed, Oct 19, 2016 at 12:40 AM, Vladimir Zapolskiy <vz@mleia.com> 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

Re: [PATCH] pinctrl: imx: reset group index on probe

[Vladimir Zapolskiy]

Hi Linus, Stefan,

On 25.10.2016 12:11, Linus Walleij wrote:
> On Wed, Oct 19, 2016 at 12:40 AM, Vladimir Zapolskiy <vz@mleia.com> 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