diff mbox

[next] pinctrl: pinctrl-single: add allocation failure checking of saved_vals

Message ID 20180606134338.4645-1-colin.king@canonical.com (mailing list archive)
State New, archived
Headers show

Commit Message

Colin King June 6, 2018, 1:43 p.m. UTC 
From: Colin Ian King <colin.king@canonical.com>

Currently saved_vals is being allocated and there is no check for
failed allocation (which is more likely than normal when using
GFP_ATOMIC).  Fix this by checking for a failed allocation and
propagating this error return down the the caller chain.

Detected by CoverityScan, CID#1469841 ("Dereference null return value")

Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Andy Shevchenko June 6, 2018, 4:02 p.m. UTC | #1 
On Wed, Jun 6, 2018 at 4:43 PM, Colin King <colin.king@canonical.com> wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently saved_vals is being allocated and there is no check for
> failed allocation (which is more likely than normal when using
> GFP_ATOMIC).  Fix this by checking for a failed allocation and
> propagating this error return down the the caller chain.
>
> Detected by CoverityScan, CID#1469841 ("Dereference null return value")
>
> Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 9c3c00515aa0..0905ee002041 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -1588,8 +1588,11 @@ static int pcs_save_context(struct pcs_device *pcs)
>
>         mux_bytes = pcs->width / BITS_PER_BYTE;
>
> -       if (!pcs->saved_vals)
> +       if (!pcs->saved_vals) {
>                 pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);

> +               if (!pcs->saved_vals)
> +                       return -ENOMEM;

Wouldn't make sense to move it out of the first condition?

Something like

if (!foo)
 foo = ...malloc(...);
if (!foo)
 return ...


> +       }
>
>         switch (pcs->width) {
>         case 64:
> @@ -1649,8 +1652,13 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
>         if (!pcs)
>                 return -EINVAL;
>
> -       if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
> -               pcs_save_context(pcs);
> +       if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
> +               int ret;
> +
> +               ret = pcs_save_context(pcs);
> +               if (ret < 0)
> +                       return ret;
> +       }
>
>         return pinctrl_force_sleep(pcs->pctl);
>  }
Johan Hovold June 7, 2018, 7:29 a.m. UTC | #2 
On Wed, Jun 06, 2018 at 02:43:38PM +0100, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> Currently saved_vals is being allocated and there is no check for
> failed allocation (which is more likely than normal when using
> GFP_ATOMIC).  Fix this by checking for a failed allocation and
> propagating this error return down the the caller chain.
> 
> Detected by CoverityScan, CID#1469841 ("Dereference null return value")
> Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> index 9c3c00515aa0..0905ee002041 100644
> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -1588,8 +1588,11 @@ static int pcs_save_context(struct pcs_device *pcs)
>  
>  	mux_bytes = pcs->width / BITS_PER_BYTE;
>  
> -	if (!pcs->saved_vals)
> +	if (!pcs->saved_vals) {
>  		pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);
> +		if (!pcs->saved_vals)
> +			return -ENOMEM;
> +	}
>  
>  	switch (pcs->width) {
>  	case 64:
> @@ -1649,8 +1652,13 @@ static int pinctrl_single_suspend(struct platform_device *pdev,
>  	if (!pcs)
>  		return -EINVAL;
>  
> -	if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
> -		pcs_save_context(pcs);
> +	if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
> +		int ret;
> +
> +		ret = pcs_save_context(pcs);
> +		if (ret < 0)
> +			return ret;
> +	}

This appears to be the right fix (along the lines of what the author may
have intended by having the helper return an int), but as a follow-up
patch, why not move the allocation to probe() instead?

Also this doesn't look like something that requires atomic allocation in
the first place, GFP_KERNEL should do for the legacy suspend callback.

>  	return pinctrl_force_sleep(pcs->pctl);
>  }

But for this fix, feel free to add:

Reviewed-by: Johan Hovold <johan@kernel.org>

Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johan Hovold June 7, 2018, 7:35 a.m. UTC | #3 
On Wed, Jun 06, 2018 at 07:02:03PM +0300, Andy Shevchenko wrote:
> On Wed, Jun 6, 2018 at 4:43 PM, Colin King <colin.king@canonical.com> wrote:
> > From: Colin Ian King <colin.king@canonical.com>
> >
> > Currently saved_vals is being allocated and there is no check for
> > failed allocation (which is more likely than normal when using
> > GFP_ATOMIC).  Fix this by checking for a failed allocation and
> > propagating this error return down the the caller chain.
> >
> > Detected by CoverityScan, CID#1469841 ("Dereference null return value")
> >
> > Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
> > Signed-off-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
> > index 9c3c00515aa0..0905ee002041 100644
> > --- a/drivers/pinctrl/pinctrl-single.c
> > +++ b/drivers/pinctrl/pinctrl-single.c
> > @@ -1588,8 +1588,11 @@ static int pcs_save_context(struct pcs_device *pcs)
> >
> >         mux_bytes = pcs->width / BITS_PER_BYTE;
> >
> > -       if (!pcs->saved_vals)
> > +       if (!pcs->saved_vals) {
> >                 pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);
> 
> > +               if (!pcs->saved_vals)
> > +                       return -ENOMEM;
> 
> Wouldn't make sense to move it out of the first condition?
> 
> Something like
> 
> if (!foo)
>  foo = ...malloc(...);
> if (!foo)
>  return ...

No, checking for NULL immediately after the allocation is more obvious
and easier to parse.

Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Colin King June 7, 2018, 8:26 a.m. UTC | #4 
On 07/06/18 08:35, Johan Hovold wrote:
> On Wed, Jun 06, 2018 at 07:02:03PM +0300, Andy Shevchenko wrote:
>> On Wed, Jun 6, 2018 at 4:43 PM, Colin King <colin.king@canonical.com> wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> Currently saved_vals is being allocated and there is no check for
>>> failed allocation (which is more likely than normal when using
>>> GFP_ATOMIC).  Fix this by checking for a failed allocation and
>>> propagating this error return down the the caller chain.
>>>
>>> Detected by CoverityScan, CID#1469841 ("Dereference null return value")
>>>
>>> Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>  drivers/pinctrl/pinctrl-single.c | 14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
>>> index 9c3c00515aa0..0905ee002041 100644
>>> --- a/drivers/pinctrl/pinctrl-single.c
>>> +++ b/drivers/pinctrl/pinctrl-single.c
>>> @@ -1588,8 +1588,11 @@ static int pcs_save_context(struct pcs_device *pcs)
>>>
>>>         mux_bytes = pcs->width / BITS_PER_BYTE;
>>>
>>> -       if (!pcs->saved_vals)
>>> +       if (!pcs->saved_vals) {
>>>                 pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);
>>
>>> +               if (!pcs->saved_vals)
>>> +                       return -ENOMEM;
>>
>> Wouldn't make sense to move it out of the first condition?
>>
>> Something like
>>
>> if (!foo)
>>  foo = ...malloc(...);
>> if (!foo)
>>  return ...
> 
> No, checking for NULL immediately after the allocation is more obvious
> and easier to parse.

+1 on that
> 
> Johan
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tony Lindgren June 8, 2018, 6:23 a.m. UTC | #5 
* Johan Hovold <johan@kernel.org> [180607 07:32]:
> On Wed, Jun 06, 2018 at 02:43:38PM +0100, Colin King wrote:
> 
> But for this fix, feel free to add:
> 
> Reviewed-by: Johan Hovold <johan@kernel.org>

Acked-by: Tony Lindgren <tony@atomide.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 14, 2018, 8:31 a.m. UTC | #6 
On Wed, Jun 6, 2018 at 3:43 PM, Colin King <colin.king@canonical.com> wrote:

> From: Colin Ian King <colin.king@canonical.com>
>
> Currently saved_vals is being allocated and there is no check for
> failed allocation (which is more likely than normal when using
> GFP_ATOMIC).  Fix this by checking for a failed allocation and
> propagating this error return down the the caller chain.
>
> Detected by CoverityScan, CID#1469841 ("Dereference null return value")
>
> Fixes: 88a1dbdec682 ("pinctrl: pinctrl-single: Add functions to save and restore pinctrl context")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>

Patch applied with Johan's and Tony's ACKs.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 9c3c00515aa0..0905ee002041 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1588,8 +1588,11 @@  static int pcs_save_context(struct pcs_device *pcs)
 
 	mux_bytes = pcs->width / BITS_PER_BYTE;
 
-	if (!pcs->saved_vals)
+	if (!pcs->saved_vals) {
 		pcs->saved_vals = devm_kzalloc(pcs->dev, pcs->size, GFP_ATOMIC);
+		if (!pcs->saved_vals)
+			return -ENOMEM;
+	}
 
 	switch (pcs->width) {
 	case 64:
@@ -1649,8 +1652,13 @@  static int pinctrl_single_suspend(struct platform_device *pdev,
 	if (!pcs)
 		return -EINVAL;
 
-	if (pcs->flags & PCS_CONTEXT_LOSS_OFF)
-		pcs_save_context(pcs);
+	if (pcs->flags & PCS_CONTEXT_LOSS_OFF) {
+		int ret;
+
+		ret = pcs_save_context(pcs);
+		if (ret < 0)
+			return ret;
+	}
 
 	return pinctrl_force_sleep(pcs->pctl);
 }