pinctrl: mvebu: prevent walking off the end of group array

Commit Message

Jason Cooper March 13, 2013, 5:35 p.m. UTC

From: David Woodhouse <[e-mail]>

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 <[e-mail]>
Signed-off-by: Jason Cooper <[e-mail]>
---
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(-)

Comments

Linus Walleij March 13, 2013, 5:47 p.m. UTC | #1

On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <[e-mail]> wrote:

> From: David Woodhouse <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Jason Cooper <[e-mail]>
> ---
> 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

Sebastian Hesselbarth March 13, 2013, 5:48 p.m. UTC | #2

On 03/13/2013 06:35 PM, Jason Cooper wrote:
> From: David Woodhouse<[e-mail]>
>
> 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<[e-mail]>
> Signed-off-by: Jason Cooper<[e-mail]>
> ---
> 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

Jason Cooper March 13, 2013, 5:50 p.m. UTC | #3

On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <[e-mail]> wrote:
> 
> > From: David Woodhouse <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Jason Cooper <[e-mail]>
> > ---
> > 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.

Linus Walleij March 13, 2013, 6:40 p.m. UTC | #4

On Wed, Mar 13, 2013 at 6:50 PM, Jason Cooper <[e-mail]> 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 <[e-mail]> wrote:
>>
>> > From: David Woodhouse <[e-mail]>
>> >
>> > 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 <[e-mail]>
>> > Signed-off-by: Jason Cooper <[e-mail]>
>> > ---
>> > 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

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;