devm_kasprintf() may return NULL on failure of internal allocation
thus the assignments are not safe if not checked. On error
rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
negative values so -ENOMEM in the (unlikely) failure case of
devm_kasprintf() should be fine here.
Signed-off-by: Nicholas Mc Guire <[email protected]>
Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
---
Problem was located with an experimental coccinelle script
The dev_err() for this unlikely case is added so that a failures of
rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
understood as currently the negative return value is simply passed up
the call stack but it would be hard to identify the cause.
Patch was compile tested with: shmobile_defconfig (implies PINCTRL_RZA1=y)
Patch is against 4.20-rc3 (localversion-next is next-20181123)
drivers/pinctrl/pinctrl-rza1.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
index 14eb576..2533df4 100644
--- a/drivers/pinctrl/pinctrl-rza1.c
+++ b/drivers/pinctrl/pinctrl-rza1.c
@@ -1225,6 +1225,11 @@ static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
chip->base = -1;
chip->label = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pOFn",
np);
+ if (!chip->label) {
+ dev_err(rza1_pctl->dev, "Failed to allocate label\n");
+ return -ENOMEM;
+ }
+
chip->ngpio = of_args.args[2];
chip->of_node = np;
chip->parent = rza1_pctl->dev;
@@ -1326,6 +1331,11 @@ static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
pins[i].number = i;
pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
"P%u-%u", port, pin);
+ if (!pins[i].name) {
+ dev_err(rza1_pctl->dev,
+ "RZ/A1 pin controller allocation failed\n");
+ return -ENOMEM;
+ }
if (i % RZA1_PINS_PER_PORT == 0) {
/*
--
2.1.4
On Sat, Nov 24, 2018 at 5:34 PM Nicholas Mc Guire <[email protected]> wrote:
> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
This patch needs to be handled by Geert Uytterhoeven,
who maintain the Renesas pin controllers.
If he didn't get it you might have to resend the patch to him.
Yours,
Linus Walleij
Hi Nicholas, Linus,
I'm sorry I might have missed this
On Sat, Nov 24, 2018 at 05:30:33PM +0100, Nicholas Mc Guire wrote:
> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.
devm_kasprintf() returns -ENOMEM in case of failure (which I agree it's
unlikely, but still...), so I guess it's fine returning -ENOMEM as well
here.
Acked-by: Jacopo Mondi <[email protected]>
Thanks
j
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
> ---
>
> Problem was located with an experimental coccinelle script
>
> The dev_err() for this unlikely case is added so that a failures of
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
> understood as currently the negative return value is simply passed up
> the call stack but it would be hard to identify the cause.
>
> Patch was compile tested with: shmobile_defconfig (implies PINCTRL_RZA1=y)
>
> Patch is against 4.20-rc3 (localversion-next is next-20181123)
>
> drivers/pinctrl/pinctrl-rza1.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
> index 14eb576..2533df4 100644
> --- a/drivers/pinctrl/pinctrl-rza1.c
> +++ b/drivers/pinctrl/pinctrl-rza1.c
> @@ -1225,6 +1225,11 @@ static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
> chip->base = -1;
> chip->label = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%pOFn",
> np);
> + if (!chip->label) {
> + dev_err(rza1_pctl->dev, "Failed to allocate label\n");
> + return -ENOMEM;
> + }
> +
> chip->ngpio = of_args.args[2];
> chip->of_node = np;
> chip->parent = rza1_pctl->dev;
> @@ -1326,6 +1331,11 @@ static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
> pins[i].number = i;
> pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
> "P%u-%u", port, pin);
> + if (!pins[i].name) {
> + dev_err(rza1_pctl->dev,
> + "RZ/A1 pin controller allocation failed\n");
> + return -ENOMEM;
> + }
>
> if (i % RZA1_PINS_PER_PORT == 0) {
> /*
> --
> 2.1.4
>
Hi Nicholas,
On Sat, Nov 24, 2018 at 5:35 PM Nicholas Mc Guire <[email protected]> wrote:
> devm_kasprintf() may return NULL on failure of internal allocation
> thus the assignments are not safe if not checked. On error
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() return
> negative values so -ENOMEM in the (unlikely) failure case of
> devm_kasprintf() should be fine here.
>
> Signed-off-by: Nicholas Mc Guire <[email protected]>
> Fixes: 5a49b644b307 ("pinctrl: Renesas RZ/A1 pin and gpio controller")
Thanks for your patch!
> The dev_err() for this unlikely case is added so that a failures of
> rza1_pinctrl_register() respectively rza1_parse_gpiochip() can be
> understood as currently the negative return value is simply passed up
> the call stack but it would be hard to identify the cause.
Adding the dev_err() is not needed, as the memory management core
print a message on memory allocation failures anyway.
Hence please remove it.
With the above fixed:
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds