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
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
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 <[email protected]>
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
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 <[email protected]> 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 <[email protected]>
>
> 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
>
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 <[email protected]>
>
> 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
>
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
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
>
On Sun, Mar 10, 2013 at 1:06 AM, Jason Cooper <[email protected]> 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
From: David Woodhouse <[email protected]>
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 <[email protected]>
Signed-off-by: Jason Cooper <[email protected]>
---
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
On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <[email protected]> wrote:
> From: David Woodhouse <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jason Cooper <[email protected]>
> ---
> 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
From: David Woodhouse <[email protected]>
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 <[email protected]>
Signed-off-by: Jason Cooper <[email protected]>
---
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
On 03/13/2013 06:35 PM, Jason Cooper wrote:
> From: David Woodhouse<[email protected]>
>
> 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<[email protected]>
> Signed-off-by: Jason Cooper<[email protected]>
> ---
> 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
On Wed, Mar 13, 2013 at 06:47:36PM +0100, Linus Walleij wrote:
> On Wed, Mar 13, 2013 at 6:35 PM, Jason Cooper <[email protected]> wrote:
>
> > From: David Woodhouse <[email protected]>
> >
> > 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 <[email protected]>
> > Signed-off-by: Jason Cooper <[email protected]>
> > ---
> > 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.
On Wed, Mar 13, 2013 at 6:50 PM, Jason Cooper <[email protected]> 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 <[email protected]> wrote:
>>
>> > From: David Woodhouse <[email protected]>
>> >
>> > 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 <[email protected]>
>> > Signed-off-by: Jason Cooper <[email protected]>
>> > ---
>> > 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
On Wed, 2013-03-13 at 17:48 +0000, Jason Cooper wrote:
> From: David Woodhouse <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jason Cooper <[email protected]>
> ---
> 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
[email protected] Intel Corporation
From: David Woodhouse <[email protected]>
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 <[email protected]>
Signed-off-by: Jason Cooper <[email protected]>
Signed-off-by: Sebastian Hesselbarth <[email protected]>
---
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 <[email protected]>
Cc: David Woodhouse <[email protected]>
Cc: Sebastian Hesselbarth <[email protected]>
Cc: Thomas Petazzoni <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Ezequiel Garcia <[email protected]>
Cc: Linus Walleij <[email protected]>
Cc: Stephen Warren <[email protected]>
Cc: Linux ARM Kernel <[email protected]>
Cc: [email protected]
---
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
Linus,
On Sat, Mar 16, 2013 at 12:44:32PM +0100, Sebastian Hesselbarth wrote:
> From: David Woodhouse <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jason Cooper <[email protected]>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
> ---
> 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 <[email protected]>
thx,
Jason.
> ---
> Cc: Jason Cooper <[email protected]>
> Cc: David Woodhouse <[email protected]>
> Cc: Sebastian Hesselbarth <[email protected]>
> Cc: Thomas Petazzoni <[email protected]>
> Cc: Gregory Clement <[email protected]>
> Cc: Andrew Lunn <[email protected]>
> Cc: Ezequiel Garcia <[email protected]>
> Cc: Linus Walleij <[email protected]>
> Cc: Stephen Warren <[email protected]>
> Cc: Linux ARM Kernel <[email protected]>
> Cc: [email protected]
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sat, Mar 16, 2013 at 12:44 PM, Sebastian Hesselbarth
<[email protected]> wrote:
> From: David Woodhouse <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Jason Cooper <[email protected]>
> Signed-off-by: Sebastian Hesselbarth <[email protected]>
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
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
[email protected] Intel Corporation
On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
<[email protected]> 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
On Wed, Mar 27, 2013 at 11:34:53PM +0100, Linus Walleij wrote:
> On Wed, Mar 27, 2013 at 11:11 PM, Woodhouse, David
> <[email protected]> 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.