memory leak and other oddness in pinctrl-mvebu.c

memory leak and other oddness in pinctrl-mvebu.c

[David Woodhouse]

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

I'm just looking through the kernel for krealloc() abuse, and the
'interesting' code in mvebu_pinctrl_build_functions() came to my
attention.

First it allocates an array 'funcs' as follows:
	/* we allocate functions for number of pins and hope
	 * there are less unique functions than pins available */
	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);

(note: s/less/fewer/ in the comment).

Then it populates the array, seemly trusting to its "hope" and just
going off the end of the array if there are more unique functions than
pins?

And then finally it reallocates the array to the correct size:

	/* with the number of unique functions and it's groups known,
	   reallocate functions and assign group names */
	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
			 GFP_KERNEL);

(note: s/it's/its/)

So... if krealloc fails, we *leak* the originally-allocated 'funcs'.
Except actually we don't because it was allocated with devm_kzalloc() so
that's OK.

If krealloc *succeeds* then I think we should get a double-free because
it doesn't free the old 'funcs' via devm_kfree() so when the device is
eventually released, won't devres_release_all() free it again?

I'm not entirely sure what that krealloc is *for*, anyway. Apart from
retrospectively reallocating the array to the size that we've already
*scribbled* over? Some kind of request for forgiveness, perhaps?

We should probably make the standard kfree() (and hence krealloc())
actually fail when handed pointers which were allocated with
devm_kzalloc()?

This completely untested patch attempts to fix it by counting the
maximum number of functions in advance, then allocating the array at
that size. In practice it may overallocate if there are name collisions
and _add_function() returns -EEXIST. Is that something we really need to
lose sleep over?

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..5f9ece6 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -500,13 +500,25 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int num = 0;
 	int n, s;
 
-	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
-	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+	/* Count the number of functions prior to allocating 'funcs' array */
+	for (n = 0; n < pctl->num_groups; n++) {
+		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
+		for (s = 0; s < grp->num_settings; s++) {
+			/* skip unsupported settings on this variant */
+			if (pctl->variant &&
+			    !(pctl->variant & grp->settings[s].variant))
+				continue;
+
+			num++;
+		}
+	}
+
+	funcs = devm_kzalloc(&pdev->dev, num *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
 		return -ENOMEM;
 
+	num = 0;
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
@@ -523,13 +535,6 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 

-- 
dwmw2

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: memory leak and other oddness in pinctrl-mvebu.c

[Sebastian Hesselbarth]

On 03/08/2013 04:58 PM, David Woodhouse wrote:
> I'm just looking through the kernel for krealloc() abuse, and the
> 'interesting' code in mvebu_pinctrl_build_functions() came to my
> attention.
>
> First it allocates an array 'funcs' as follows:
> 	/* we allocate functions for number of pins and hope
> 	 * there are less unique functions than pins available */
> 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
> 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>
> (note: s/less/fewer/ in the comment).
>
> Then it populates the array, seemly trusting to its "hope" and just
> going off the end of the array if there are more unique functions than
> pins?
>
> And then finally it reallocates the array to the correct size:
>
> 	/* with the number of unique functions and it's groups known,
> 	   reallocate functions and assign group names */
> 	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> 			 GFP_KERNEL);
>
> (note: s/it's/its/)
>
> So... if krealloc fails, we *leak* the originally-allocated 'funcs'.
> Except actually we don't because it was allocated with devm_kzalloc() so
> that's OK.

David,

the purpose of the above code is to build up a list of unique functions
and the pins supporting this function. This is a requirement of the pinctrl
API and we had a lengthy discussion with Steven and LinusW back then as
the hardware works just the other way round, i.e. a list of pins with a
set of functions.

The idea was to first assume there will be no more unique functions than
pins available and use that array to store the unique function names. After
that shrink the array to the actual number of unique functions. BTW, this
array isn't used within the driver at all, except to fulfill API requirements
for debugfs.

> If krealloc *succeeds* then I think we should get a double-free because
> it doesn't free the old 'funcs' via devm_kfree() so when the device is
> eventually released, won't devres_release_all() free it again?

I understand that using devm_kzalloc and krealloc will lead to either
double-free or ignored krealloc failure. Is there any other/more correct
way of reallocating that pointer? The fix I would prefer is to allocate
the array but not realloc it to the correct size.

> I'm not entirely sure what that krealloc is *for*, anyway. Apart from
> retrospectively reallocating the array to the size that we've already
> *scribbled* over? Some kind of request for forgiveness, perhaps?
>
> We should probably make the standard kfree() (and hence krealloc())
> actually fail when handed pointers which were allocated with
> devm_kzalloc()?
>
> This completely untested patch attempts to fix it by counting the
> maximum number of functions in advance, then allocating the array at
> that size. In practice it may overallocate if there are name collisions
> and _add_function() returns -EEXIST. Is that something we really need to
> lose sleep over?

I agree that counting all potential unique functions is safe here, but
will lead to overallocation for sure. Actually, the number of unique functions
is known in advance for each SoC but first the information is already there
in what the driver uses for controlling the HW (pins with functions) but with
multiarch kernels in mind, we chose to built that list on runtime.

I don't have a strong opinion on that, but I prefer not to have the list
statically in the SoC specific drivers. I think counting the number of
unique functions for each SoC specific driver once and verify the above
heuristic (fewer unique functions than pins) is still valid. Then drop
the krealloc and leave the array the way it is allocated on devm_kzalloc.

Sebastian

Re: memory leak and other oddness in pinctrl-mvebu.c

[David Woodhouse]

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

On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
> I don't have a strong opinion on that, but I prefer not to have the list
> statically in the SoC specific drivers. I think counting the number of
> unique functions for each SoC specific driver once and verify the above
> heuristic (fewer unique functions than pins) is still valid. Then drop
> the krealloc and leave the array the way it is allocated on devm_kzalloc.

Yeah. If you stick a check in the loop and make it warn if it *would*
have run over the end of the array, that sounds like it ought to be
fine. Something like this, perhaps? Still untested but otherwise
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..55d55d5 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+			 const char *name)
 {
 	while (funcs->num_groups) {
 		/* function already there */
@@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
+			if (ret)
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 


-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: memory leak and other oddness in pinctrl-mvebu.c

[Sebastian Hesselbarth]

David,

I will not be able to test before mid-week ealiest. I added Andrew Lunn to
the list. He and Thomas can test your patch for Kirkwood and Armada XP/370
respectively. I will test on Dove asap.

Sebastian

On Sat, Mar 9, 2013 at 8:02 PM, David Woodhouse <dwmw2@infradead.org> wrote:
> On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
>> I don't have a strong opinion on that, but I prefer not to have the list
>> statically in the SoC specific drivers. I think counting the number of
>> unique functions for each SoC specific driver once and verify the above
>> heuristic (fewer unique functions than pins) is still valid. Then drop
>> the krealloc and leave the array the way it is allocated on devm_kzalloc.
>
> Yeah. If you stick a check in the loop and make it warn if it *would*
> have run over the end of the array, that sounds like it ought to be
> fine. Something like this, perhaps? Still untested but otherwise
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..55d55d5 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>         .dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> +                        const char *name)
>  {
>         while (funcs->num_groups) {
>                 /* function already there */
> @@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
>                         return -EEXIST;
>                 }
>                 funcs++;
> +               nr_funcs--;
>         }
> +       if (!nr_funcs)
> +               return -EOVERFLOW;
> +
>         funcs->name = name;
>         funcs->num_groups = 1;
>         return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>         int n, s;
>
>         /* we allocate functions for number of pins and hope
> -        * there are less unique functions than pins available */
> +        * there are fewer unique functions than pins available */
>         funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
>                              sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>         if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>         for (n = 0; n < pctl->num_groups; n++) {
>                 struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>                 for (s = 0; s < grp->num_settings; s++) {
> +                       int ret;
> +
>                         /* skip unsupported settings on this variant */
>                         if (pctl->variant &&
>                             !(pctl->variant & grp->settings[s].variant))
>                                 continue;
>
>                         /* check for unique functions and count groups */
> -                       if (_add_function(funcs, grp->settings[s].name))
> +                       ret = _add_function(funcs, pctl->desc.npins,
> +                                           grp->settings[s].name);
> +                       if (ret == -EOVERFLOW)
> +                               dev_err(&pdev->dev,
> +                                       "More functions than pins(%d)\n",
> +                                       pctl->desc.npins);
> +                       if (ret)
>                                 continue;
>
>                         num++;
>                 }
>         }
>
> -       /* with the number of unique functions and it's groups known,
> -          reallocate functions and assign group names */
> -       funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -                        GFP_KERNEL);
> -       if (!funcs)
> -               return -ENOMEM;
> -
>         pctl->num_functions = num;
>         pctl->functions = funcs;
>
>
>
> --
> dwmw2
>

Re: memory leak and other oddness in pinctrl-mvebu.c

[Jason Cooper]

On Sat, Mar 09, 2013 at 07:02:05PM +0000, David Woodhouse wrote:
> On Sat, 2013-03-09 at 09:49 +0100, Sebastian Hesselbarth wrote:
> > I don't have a strong opinion on that, but I prefer not to have the list
> > statically in the SoC specific drivers. I think counting the number of
> > unique functions for each SoC specific driver once and verify the above
> > heuristic (fewer unique functions than pins) is still valid. Then drop
> > the krealloc and leave the array the way it is allocated on devm_kzalloc.
> 
> Yeah. If you stick a check in the loop and make it warn if it *would*
> have run over the end of the array, that sounds like it ought to be
> fine. Something like this, perhaps? Still untested but otherwise
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..55d55d5 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,7 +478,8 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>  	.dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>  
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> +			 const char *name)
>  {
>  	while (funcs->num_groups) {
>  		/* function already there */
> @@ -487,7 +488,11 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
>  			return -EEXIST;
>  		}
>  		funcs++;
> +		nr_funcs--;
>  	}
> +	if (!nr_funcs)

shouldn't this be:

	if (nr_funcs <= 0)

thx,

Jason.

> +		return -EOVERFLOW;
> +
>  	funcs->name = name;
>  	funcs->num_groups = 1;
>  	return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	int n, s;
>  
>  	/* we allocate functions for number of pins and hope
> -	 * there are less unique functions than pins available */
> +	 * there are fewer unique functions than pins available */
>  	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
>  			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>  	if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	for (n = 0; n < pctl->num_groups; n++) {
>  		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>  		for (s = 0; s < grp->num_settings; s++) {
> +			int ret;
> +
>  			/* skip unsupported settings on this variant */
>  			if (pctl->variant &&
>  			    !(pctl->variant & grp->settings[s].variant))
>  				continue;
>  
>  			/* check for unique functions and count groups */
> -			if (_add_function(funcs, grp->settings[s].name))
> +			ret = _add_function(funcs, pctl->desc.npins,
> +					    grp->settings[s].name);
> +			if (ret == -EOVERFLOW)
> +				dev_err(&pdev->dev,
> +					"More functions than pins(%d)\n",
> +					pctl->desc.npins);
> +			if (ret)
>  				continue;
>  
>  			num++;
>  		}
>  	}
>  
> -	/* with the number of unique functions and it's groups known,
> -	   reallocate functions and assign group names */
> -	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -			 GFP_KERNEL);
> -	if (!funcs)
> -		return -ENOMEM;
> -
>  	pctl->num_functions = num;
>  	pctl->functions = funcs;
>  
> 
> 
> -- 
> dwmw2
> 

Re: memory leak and other oddness in pinctrl-mvebu.c

[David Woodhouse]

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

On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
> > +     if (!nr_funcs)
> 
> shouldn't this be:
> 
>         if (nr_funcs <= 0)

Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
otherwise funcs->num_groups will be off the end of the original array:

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8bbc607 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+			 const char *name)
 {
-	while (funcs->num_groups) {
+	while (nr_funcs && funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
 			funcs->num_groups++;
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
+			if (ret)
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 

-- 
dwmw2


[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

memory leak and other oddness in pinctrl-mvebu.c

[Jason Cooper]

Added LinusW, Gregory and Ezequiel to the email.  Guys, can you give
this a Tested-by before I apply (or Ack for LinusW)?

thx,

Jason.

On Sat, Mar 09, 2013 at 11:39:31PM +0000, David Woodhouse wrote:
> On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
> > > +     if (!nr_funcs)
> > 
> > shouldn't this be:
> > 
> >         if (nr_funcs <= 0)
> 
> Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
> otherwise funcs->num_groups will be off the end of the original array:
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..8bbc607 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>  	.dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>  
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> +			 const char *name)
>  {
> -	while (funcs->num_groups) {
> +	while (nr_funcs && funcs->num_groups) {
>  		/* function already there */
>  		if (strcmp(funcs->name, name) == 0) {
>  			funcs->num_groups++;
>  			return -EEXIST;
>  		}
>  		funcs++;
> +		nr_funcs--;
>  	}
> +	if (!nr_funcs)
> +		return -EOVERFLOW;
> +
>  	funcs->name = name;
>  	funcs->num_groups = 1;
>  	return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	int n, s;
>  
>  	/* we allocate functions for number of pins and hope
> -	 * there are less unique functions than pins available */
> +	 * there are fewer unique functions than pins available */
>  	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
>  			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>  	if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	for (n = 0; n < pctl->num_groups; n++) {
>  		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>  		for (s = 0; s < grp->num_settings; s++) {
> +			int ret;
> +
>  			/* skip unsupported settings on this variant */
>  			if (pctl->variant &&
>  			    !(pctl->variant & grp->settings[s].variant))
>  				continue;
>  
>  			/* check for unique functions and count groups */
> -			if (_add_function(funcs, grp->settings[s].name))
> +			ret = _add_function(funcs, pctl->desc.npins,
> +					    grp->settings[s].name);
> +			if (ret == -EOVERFLOW)
> +				dev_err(&pdev->dev,
> +					"More functions than pins(%d)\n",
> +					pctl->desc.npins);
> +			if (ret)
>  				continue;
>  
>  			num++;
>  		}
>  	}
>  
> -	/* with the number of unique functions and it's groups known,
> -	   reallocate functions and assign group names */
> -	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -			 GFP_KERNEL);
> -	if (!funcs)
> -		return -ENOMEM;
> -
>  	pctl->num_functions = num;
>  	pctl->functions = funcs;
>  
> 
> -- 
> dwmw2
> 

Re: memory leak and other oddness in pinctrl-mvebu.c

[Jason Cooper]

Added LinusW, Gregory and Ezequiel to the email.  Guys, can you give
this a Tested-by before I apply (or Ack for LinusW)?

thx,

Jason.

On Sat, Mar 09, 2013 at 11:39:31PM +0000, David Woodhouse wrote:
> On Sat, 2013-03-09 at 17:53 -0500, Jason Cooper wrote:
> > > +     if (!nr_funcs)
> > 
> > shouldn't this be:
> > 
> >         if (nr_funcs <= 0)
> 
> Hm, no. But the loop should terminate if nr_funcs ever reaches zero,
> otherwise funcs->num_groups will be off the end of the original array:
> 
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..8bbc607 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>  	.dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>  
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
> +			 const char *name)
>  {
> -	while (funcs->num_groups) {
> +	while (nr_funcs && funcs->num_groups) {
>  		/* function already there */
>  		if (strcmp(funcs->name, name) == 0) {
>  			funcs->num_groups++;
>  			return -EEXIST;
>  		}
>  		funcs++;
> +		nr_funcs--;
>  	}
> +	if (!nr_funcs)
> +		return -EOVERFLOW;
> +
>  	funcs->name = name;
>  	funcs->num_groups = 1;
>  	return 0;
> @@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	int n, s;
>  
>  	/* we allocate functions for number of pins and hope
> -	 * there are less unique functions than pins available */
> +	 * there are fewer unique functions than pins available */
>  	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
>  			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>  	if (!funcs)
> @@ -510,26 +515,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	for (n = 0; n < pctl->num_groups; n++) {
>  		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>  		for (s = 0; s < grp->num_settings; s++) {
> +			int ret;
> +
>  			/* skip unsupported settings on this variant */
>  			if (pctl->variant &&
>  			    !(pctl->variant & grp->settings[s].variant))
>  				continue;
>  
>  			/* check for unique functions and count groups */
> -			if (_add_function(funcs, grp->settings[s].name))
> +			ret = _add_function(funcs, pctl->desc.npins,
> +					    grp->settings[s].name);
> +			if (ret == -EOVERFLOW)
> +				dev_err(&pdev->dev,
> +					"More functions than pins(%d)\n",
> +					pctl->desc.npins);
> +			if (ret)
>  				continue;
>  
>  			num++;
>  		}
>  	}
>  
> -	/* with the number of unique functions and it's groups known,
> -	   reallocate functions and assign group names */
> -	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -			 GFP_KERNEL);
> -	if (!funcs)
> -		return -ENOMEM;
> -
>  	pctl->num_functions = num;
>  	pctl->functions = funcs;
>  
> 
> -- 
> dwmw2
> 

memory leak and other oddness in pinctrl-mvebu.c

[Linus Walleij]

On Sun, Mar 10, 2013 at 1:06 AM, Jason Cooper <jason@lakedaemon.net> wrote:

> Added LinusW, Gregory and Ezequiel to the email.  Guys, can you give
> this a Tested-by before I apply (or Ack for LinusW)?

If the Marvell people are happy I'm happy. But I would like to have
some Tested-by from Sebastian or so? Also I don't quite see the
final form of this patch?

And shouldn't I rather take the final patch into the pinctrl tree?

Then there is a larger issue lurking behind this, and that is that
all platforms run their own awkward DT parse code, and that comes
from the fact that we could not agree on a common mapping.
We must think real hard if we can't atleast share some of the
parsing code here.

Yours,
Linus Walleij

Re: memory leak and other oddness in pinctrl-mvebu.c

[Linus Walleij]

On Sun, Mar 10, 2013 at 1:06 AM, Jason Cooper <jason@lakedaemon.net> wrote:

> Added LinusW, Gregory and Ezequiel to the email.  Guys, can you give
> this a Tested-by before I apply (or Ack for LinusW)?

If the Marvell people are happy I'm happy. But I would like to have
some Tested-by from Sebastian or so? Also I don't quite see the
final form of this patch?

And shouldn't I rather take the final patch into the pinctrl tree?

Then there is a larger issue lurking behind this, and that is that
all platforms run their own awkward DT parse code, and that comes
from the fact that we could not agree on a common mapping.
We must think real hard if we can't atleast share some of the
parsing code here.

Yours,
Linus Walleij

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8d8f175 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, nt nr_funcs,
+			const char *name)
 {
-	while (funcs->num_groups) {
+	while (nr_funcs && funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
 			funcs->num_groups++;
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,26 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.8.1.5

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..8d8f175 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, nt nr_funcs,
+			const char *name)
 {
-	while (funcs->num_groups) {
+	while (nr_funcs && funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
 			funcs->num_groups++;
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,26 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.8.1.5

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:

> From: David Woodhouse <dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
> David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.

I have tentatively applied this to my fixes branch.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:

> From: David Woodhouse <dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
> David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.

I have tentatively applied this to my fixes branch.

Yours,
Linus Walleij

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> 
> > From: David Woodhouse <dwmw2@infradead.org>
> >
> > While investigating (ab)use of krealloc, David found this bug.  It's
> > unlikely to occur, but now we detect the condition and error out
> > appropriately.
> >
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> > ---
> > David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
> 
> I have tentatively applied this to my fixes branch.

Dang, you're quick.  Please see V2 <shuffles feet>

thx,

Jason.

Re: [PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> 
> > From: David Woodhouse <dwmw2@infradead.org>
> >
> > While investigating (ab)use of krealloc, David found this bug.  It's
> > unlikely to occur, but now we detect the condition and error out
> > appropriately.
> >
> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> > ---
> > David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
> 
> I have tentatively applied this to my fixes branch.

Dang, you're quick.  Please see V2 <shuffles feet>

thx,

Jason.

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 13, 2013 at 6:50 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
>> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>>
>> > From: David Woodhouse <dwmw2@infradead.org>
>> >
>> > While investigating (ab)use of krealloc, David found this bug.  It's
>> > unlikely to occur, but now we detect the condition and error out
>> > appropriately.
>> >
>> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> > ---
>> > David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
>>
>> I have tentatively applied this to my fixes branch.
>
> Dang, you're quick.  Please see V2 <shuffles feet>

Ah, I'm too trigger happy. I took it out again waiting for some ACK
and testing..

Yours,
Linus Walleij

Re: [PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 13, 2013 at 6:50 PM, Jason Cooper <jason@lakedaemon.net> wrote:
> On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
>> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <jason@lakedaemon.net> wrote:
>>
>> > From: David Woodhouse <dwmw2@infradead.org>
>> >
>> > While investigating (ab)use of krealloc, David found this bug.  It's
>> > unlikely to occur, but now we detect the condition and error out
>> > appropriately.
>> >
>> > Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
>> > Signed-off-by: Jason Cooper <jason@lakedaemon.net>
>> > ---
>> > David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
>>
>> I have tentatively applied this to my fixes branch.
>
> Dang, you're quick.  Please see V2 <shuffles feet>

Ah, I'm too trigger happy. I took it out again waiting for some ACK
and testing..

Yours,
Linus Walleij

[PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Sebastian Hesselbarth]

On 03/13/2013 06:35 PM, Jason Cooper wrote:
> From: David Woodhouse<dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse<David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper<jason@lakedaemon.net>
> ---
> David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
>

Jason,

thanks for preparing that patch. I ll test (and Ack) asap but that will not be
before this weekend. If Thomas and Andrew ack it before me, consider it good.

Sebastian

Re: [PATCH] pinctrl: mvebu: prevent walking off the end of group array

[Sebastian Hesselbarth]

On 03/13/2013 06:35 PM, Jason Cooper wrote:
> From: David Woodhouse<dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse<David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper<jason@lakedaemon.net>
> ---
> David, please double check that this is as you intended.  I had to hand-jam it in due to some peculiarities on my side.
>

Jason,

thanks for preparing that patch. I ll test (and Ack) asap but that will not be
before this weekend. If Thomas and Andrew ack it before me, consider it good.

Sebastian

[PATCH V2] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
Changes from v1:
 - correct typo (s/ nt / int /) I should've caught before sending.

David, please double check that this is as you intended.  I had to hand-jam it
in due to some peculiarities on my side.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..5db5fad 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+			const char *name)
 {
-	while (funcs->num_groups) {
+	while (nr_funcs && funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
 			funcs->num_groups++;
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,26 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.8.1.5

[PATCH V2] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
---
Changes from v1:
 - correct typo (s/ nt / int /) I should've caught before sending.

David, please double check that this is as you intended.  I had to hand-jam it
in due to some peculiarities on my side.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..5db5fad 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,16 +478,21 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int nr_funcs,
+			const char *name)
 {
-	while (funcs->num_groups) {
+	while (nr_funcs && funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
 			funcs->num_groups++;
 			return -EEXIST;
 		}
 		funcs++;
+		nr_funcs--;
 	}
+	if (!nr_funcs)
+		return -EOVERFLOW;
+
 	funcs->name = name;
 	funcs->num_groups = 1;
 	return 0;
@@ -501,7 +506,7 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
+	 * there are fewer unique functions than pins available */
 	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
@@ -510,26 +515,26 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, pctl->desc.npins,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.8.1.5

[PATCH V2] pinctrl: mvebu: prevent walking off the end of group array

[David Woodhouse]

On Wed, 2013-03-13 at 17:48 +0000, Jason Cooper wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> 
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
> Changes from v1:
>  - correct typo (s/ nt / int /) I should've caught before sending.
> 
> David, please double check that this is as you intended.  I had to hand-jam it
> in due to some peculiarities on my side.

Looks fine; thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6171 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130314/80874e05/attachment-0001.bin>

Re: [PATCH V2] pinctrl: mvebu: prevent walking off the end of group array

[David Woodhouse]

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

On Wed, 2013-03-13 at 17:48 +0000, Jason Cooper wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> 
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> ---
> Changes from v1:
>  - correct typo (s/ nt / int /) I should've caught before sending.
> 
> David, please double check that this is as you intended.  I had to hand-jam it
> in due to some peculiarities on my side.

Looks fine; thanks.

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Sebastian Hesselbarth]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Jason, David,

I tested the patch on Dove and fixed all remaining issues.

Thomas, Gregory, Andrew should test on their platforms, too.

Sebastian

Changes from v2:
 - fix counting of available array space
 - fix return code handling

Changes from v1:
 - correct typo (s/ nt / int /) I should've caught before sending.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Linux ARM Kernel <linux-arm-kernel@lists.infradead.org>
Cc: linux-kernel at vger.kernel.org
---
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..aa77fb7a 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,8 +478,12 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int *funcsize,
+			const char *name)
 {
+	if (*funcsize <= 0)
+		return -EOVERFLOW;
+
 	while (funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
@@ -488,8 +492,12 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
 		}
 		funcs++;
 	}
+
+	/* append new unique function */
 	funcs->name = name;
 	funcs->num_groups = 1;
+	(*funcsize)--;
+
 	return 0;
 }
 
@@ -497,12 +505,12 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 					 struct mvebu_pinctrl *pctl)
 {
 	struct mvebu_pinctrl_function *funcs;
-	int num = 0;
+	int num = 0, funcsize = pctl->desc.npins;
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
-	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+	 * there are fewer unique functions than pins available */
+	funcs = devm_kzalloc(&pdev->dev, funcsize *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
 		return -ENOMEM;
@@ -510,26 +518,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, &funcsize,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
+			if (ret < 0)
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.7.10.4

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Sebastian Hesselbarth]

From: David Woodhouse <dwmw2@infradead.org>

While investigating (ab)use of krealloc, David found this bug.  It's
unlikely to occur, but now we detect the condition and error out
appropriately.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Signed-off-by: Jason Cooper <jason@lakedaemon.net>
Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
---
Jason, David,

I tested the patch on Dove and fixed all remaining issues.

Thomas, Gregory, Andrew should test on their platforms, too.

Sebastian

Changes from v2:
 - fix counting of available array space
 - fix return code handling

Changes from v1:
 - correct typo (s/ nt / int /) I should've caught before sending.

 drivers/pinctrl/mvebu/pinctrl-mvebu.c |   33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)
---
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
Cc: Stephen Warren <swarren@wwwdotorg.org>
Cc: Linux ARM Kernel <linux-arm-kernel@lists.infradead.org>
Cc: linux-kernel@vger.kernel.org
---
diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
index c689c04..aa77fb7a 100644
--- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
+++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
@@ -478,8 +478,12 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
 	.dt_free_map = mvebu_pinctrl_dt_free_map,
 };
 
-static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
+static int _add_function(struct mvebu_pinctrl_function *funcs, int *funcsize,
+			const char *name)
 {
+	if (*funcsize <= 0)
+		return -EOVERFLOW;
+
 	while (funcs->num_groups) {
 		/* function already there */
 		if (strcmp(funcs->name, name) == 0) {
@@ -488,8 +492,12 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
 		}
 		funcs++;
 	}
+
+	/* append new unique function */
 	funcs->name = name;
 	funcs->num_groups = 1;
+	(*funcsize)--;
+
 	return 0;
 }
 
@@ -497,12 +505,12 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 					 struct mvebu_pinctrl *pctl)
 {
 	struct mvebu_pinctrl_function *funcs;
-	int num = 0;
+	int num = 0, funcsize = pctl->desc.npins;
 	int n, s;
 
 	/* we allocate functions for number of pins and hope
-	 * there are less unique functions than pins available */
-	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
+	 * there are fewer unique functions than pins available */
+	funcs = devm_kzalloc(&pdev->dev, funcsize *
 			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
 	if (!funcs)
 		return -ENOMEM;
@@ -510,26 +518,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
 	for (n = 0; n < pctl->num_groups; n++) {
 		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
 		for (s = 0; s < grp->num_settings; s++) {
+			int ret;
+
 			/* skip unsupported settings on this variant */
 			if (pctl->variant &&
 			    !(pctl->variant & grp->settings[s].variant))
 				continue;
 
 			/* check for unique functions and count groups */
-			if (_add_function(funcs, grp->settings[s].name))
+			ret = _add_function(funcs, &funcsize,
+					    grp->settings[s].name);
+			if (ret == -EOVERFLOW)
+				dev_err(&pdev->dev,
+					"More functions than pins(%d)\n",
+					pctl->desc.npins);
+			if (ret < 0)
 				continue;
 
 			num++;
 		}
 	}
 
-	/* with the number of unique functions and it's groups known,
-	   reallocate functions and assign group names */
-	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
-			 GFP_KERNEL);
-	if (!funcs)
-		return -ENOMEM;
-
 	pctl->num_functions = num;
 	pctl->functions = funcs;
 
-- 
1.7.10.4

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

Linus,

On Sat, Mar 16, 2013 at 12:44:32PM +0100, Sebastian Hesselbarth wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> 
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Jason, David,
> 
> I tested the patch on Dove and fixed all remaining issues.
> 
> Thomas, Gregory, Andrew should test on their platforms, too.
> 
> Sebastian
> 
> Changes from v2:
>  - fix counting of available array space
>  - fix return code handling
> 
> Changes from v1:
>  - correct typo (s/ nt / int /) I should've caught before sending.
> 
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c |   33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)

Does this look good to you?

fwiw,

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

> ---
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Linux ARM Kernel <linux-arm-kernel@lists.infradead.org>
> Cc: linux-kernel at vger.kernel.org
> ---
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..aa77fb7a 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,8 +478,12 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>  	.dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>  
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int *funcsize,
> +			const char *name)
>  {
> +	if (*funcsize <= 0)
> +		return -EOVERFLOW;
> +
>  	while (funcs->num_groups) {
>  		/* function already there */
>  		if (strcmp(funcs->name, name) == 0) {
> @@ -488,8 +492,12 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
>  		}
>  		funcs++;
>  	}
> +
> +	/* append new unique function */
>  	funcs->name = name;
>  	funcs->num_groups = 1;
> +	(*funcsize)--;
> +
>  	return 0;
>  }
>  
> @@ -497,12 +505,12 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  					 struct mvebu_pinctrl *pctl)
>  {
>  	struct mvebu_pinctrl_function *funcs;
> -	int num = 0;
> +	int num = 0, funcsize = pctl->desc.npins;
>  	int n, s;
>  
>  	/* we allocate functions for number of pins and hope
> -	 * there are less unique functions than pins available */
> -	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
> +	 * there are fewer unique functions than pins available */
> +	funcs = devm_kzalloc(&pdev->dev, funcsize *
>  			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>  	if (!funcs)
>  		return -ENOMEM;
> @@ -510,26 +518,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	for (n = 0; n < pctl->num_groups; n++) {
>  		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>  		for (s = 0; s < grp->num_settings; s++) {
> +			int ret;
> +
>  			/* skip unsupported settings on this variant */
>  			if (pctl->variant &&
>  			    !(pctl->variant & grp->settings[s].variant))
>  				continue;
>  
>  			/* check for unique functions and count groups */
> -			if (_add_function(funcs, grp->settings[s].name))
> +			ret = _add_function(funcs, &funcsize,
> +					    grp->settings[s].name);
> +			if (ret == -EOVERFLOW)
> +				dev_err(&pdev->dev,
> +					"More functions than pins(%d)\n",
> +					pctl->desc.npins);
> +			if (ret < 0)
>  				continue;
>  
>  			num++;
>  		}
>  	}
>  
> -	/* with the number of unique functions and it's groups known,
> -	   reallocate functions and assign group names */
> -	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -			 GFP_KERNEL);
> -	if (!funcs)
> -		return -ENOMEM;
> -
>  	pctl->num_functions = num;
>  	pctl->functions = funcs;
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

Linus,

On Sat, Mar 16, 2013 at 12:44:32PM +0100, Sebastian Hesselbarth wrote:
> From: David Woodhouse <dwmw2@infradead.org>
> 
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
> 
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> ---
> Jason, David,
> 
> I tested the patch on Dove and fixed all remaining issues.
> 
> Thomas, Gregory, Andrew should test on their platforms, too.
> 
> Sebastian
> 
> Changes from v2:
>  - fix counting of available array space
>  - fix return code handling
> 
> Changes from v1:
>  - correct typo (s/ nt / int /) I should've caught before sending.
> 
>  drivers/pinctrl/mvebu/pinctrl-mvebu.c |   33 +++++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 12 deletions(-)

Does this look good to you?

fwiw,

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.

> ---
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Gregory Clement <gregory.clement@free-electrons.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Stephen Warren <swarren@wwwdotorg.org>
> Cc: Linux ARM Kernel <linux-arm-kernel@lists.infradead.org>
> Cc: linux-kernel@vger.kernel.org
> ---
> diff --git a/drivers/pinctrl/mvebu/pinctrl-mvebu.c b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> index c689c04..aa77fb7a 100644
> --- a/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> +++ b/drivers/pinctrl/mvebu/pinctrl-mvebu.c
> @@ -478,8 +478,12 @@ static struct pinctrl_ops mvebu_pinctrl_ops = {
>  	.dt_free_map = mvebu_pinctrl_dt_free_map,
>  };
>  
> -static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
> +static int _add_function(struct mvebu_pinctrl_function *funcs, int *funcsize,
> +			const char *name)
>  {
> +	if (*funcsize <= 0)
> +		return -EOVERFLOW;
> +
>  	while (funcs->num_groups) {
>  		/* function already there */
>  		if (strcmp(funcs->name, name) == 0) {
> @@ -488,8 +492,12 @@ static int _add_function(struct mvebu_pinctrl_function *funcs, const char *name)
>  		}
>  		funcs++;
>  	}
> +
> +	/* append new unique function */
>  	funcs->name = name;
>  	funcs->num_groups = 1;
> +	(*funcsize)--;
> +
>  	return 0;
>  }
>  
> @@ -497,12 +505,12 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  					 struct mvebu_pinctrl *pctl)
>  {
>  	struct mvebu_pinctrl_function *funcs;
> -	int num = 0;
> +	int num = 0, funcsize = pctl->desc.npins;
>  	int n, s;
>  
>  	/* we allocate functions for number of pins and hope
> -	 * there are less unique functions than pins available */
> -	funcs = devm_kzalloc(&pdev->dev, pctl->desc.npins *
> +	 * there are fewer unique functions than pins available */
> +	funcs = devm_kzalloc(&pdev->dev, funcsize *
>  			     sizeof(struct mvebu_pinctrl_function), GFP_KERNEL);
>  	if (!funcs)
>  		return -ENOMEM;
> @@ -510,26 +518,27 @@ static int mvebu_pinctrl_build_functions(struct platform_device *pdev,
>  	for (n = 0; n < pctl->num_groups; n++) {
>  		struct mvebu_pinctrl_group *grp = &pctl->groups[n];
>  		for (s = 0; s < grp->num_settings; s++) {
> +			int ret;
> +
>  			/* skip unsupported settings on this variant */
>  			if (pctl->variant &&
>  			    !(pctl->variant & grp->settings[s].variant))
>  				continue;
>  
>  			/* check for unique functions and count groups */
> -			if (_add_function(funcs, grp->settings[s].name))
> +			ret = _add_function(funcs, &funcsize,
> +					    grp->settings[s].name);
> +			if (ret == -EOVERFLOW)
> +				dev_err(&pdev->dev,
> +					"More functions than pins(%d)\n",
> +					pctl->desc.npins);
> +			if (ret < 0)
>  				continue;
>  
>  			num++;
>  		}
>  	}
>  
> -	/* with the number of unique functions and it's groups known,
> -	   reallocate functions and assign group names */
> -	funcs = krealloc(funcs, num * sizeof(struct mvebu_pinctrl_function),
> -			 GFP_KERNEL);
> -	if (!funcs)
> -		return -ENOMEM;
> -
>  	pctl->num_functions = num;
>  	pctl->functions = funcs;
>  
> -- 
> 1.7.10.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Sat, Mar 16, 2013 at 12:44 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

> From: David Woodhouse <dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

OK this v2 version applied for fixes...

Is it so critical that it needs to be tagged for stable as well
then tell me.

If it's non-critical enough to live on for -next then tell me.

Yours,
Linus Walleij

Re: [PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Sat, Mar 16, 2013 at 12:44 PM, Sebastian Hesselbarth
<sebastian.hesselbarth@gmail.com> wrote:

> From: David Woodhouse <dwmw2@infradead.org>
>
> While investigating (ab)use of krealloc, David found this bug.  It's
> unlikely to occur, but now we detect the condition and error out
> appropriately.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Signed-off-by: Jason Cooper <jason@lakedaemon.net>
> Signed-off-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>

OK this v2 version applied for fixes...

Is it so critical that it needs to be tagged for stable as well
then tell me.

If it's non-critical enough to live on for -next then tell me.

Yours,
Linus Walleij

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Woodhouse, David]

On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
> Is it so critical that it needs to be tagged for stable as well
> then tell me.
> 
> If it's non-critical enough to live on for -next then tell me.

I don't think it's critical. It'll probably never trigger in practice.

-- 
                   Sent with MeeGo's ActiveSync support.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 6242 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130327/30d1c6d2/attachment-0001.bin>

Re: [PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Woodhouse, David]

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

On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
> Is it so critical that it needs to be tagged for stable as well
> then tell me.
> 
> If it's non-critical enough to live on for -next then tell me.

I don't think it's critical. It'll probably never trigger in practice.

-- 
                   Sent with MeeGo's ActiveSync support.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6242 bytes --]

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
<david.woodhouse@intel.com> wrote:
> On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
>> Is it so critical that it needs to be tagged for stable as well
>> then tell me.
>>
>> If it's non-critical enough to live on for -next then tell me.
>
> I don't think it's critical. It'll probably never trigger in practice.

Thanks, moving it to for-next then...

Yours,
Linus Walleij

Re: [PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Linus Walleij]

On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
<david.woodhouse@intel.com> wrote:
> On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
>> Is it so critical that it needs to be tagged for stable as well
>> then tell me.
>>
>> If it's non-critical enough to live on for -next then tell me.
>
> I don't think it's critical. It'll probably never trigger in practice.

Thanks, moving it to for-next then...

Yours,
Linus Walleij

[PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

On Wed, Mar 27, 2013 at 11:34:53PM +0100, Linus Walleij wrote:
> On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
> <david.woodhouse@intel.com> wrote:
> > On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
> >> Is it so critical that it needs to be tagged for stable as well
> >> then tell me.
> >>
> >> If it's non-critical enough to live on for -next then tell me.
> >
> > I don't think it's critical. It'll probably never trigger in practice.
> 
> Thanks, moving it to for-next then...

Thanks, Linus.

thx,

Jason.

Re: [PATCH v3] pinctrl: mvebu: prevent walking off the end of group array

[Jason Cooper]

On Wed, Mar 27, 2013 at 11:34:53PM +0100, Linus Walleij wrote:
> On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
> <david.woodhouse@intel.com> wrote:
> > On Wed, 2013-03-27 at 23:06 +0100, Linus Walleij wrote:
> >> Is it so critical that it needs to be tagged for stable as well
> >> then tell me.
> >>
> >> If it's non-critical enough to live on for -next then tell me.
> >
> > I don't think it's critical. It'll probably never trigger in practice.
> 
> Thanks, moving it to for-next then...

Thanks, Linus.

thx,

Jason.