Re: [PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Dan Carpenter]

On Tue, Feb 05, 2019 at 11:37:40AM +0800, Chen-Yu Tsai wrote:
> On Sat, Dec 22, 2018 at 7:32 PM Colin King  wrote:
> >
> > From: Colin Ian King 
> >
> > Currently rdev is dereferenced when assigning desc before rdev is null
> > checked, hence there is a potential null pointer dereference on rdev.
> > Fix this by null checking rdev first.
> >
> > Detected by CoverityScan, CID#1476031 ("Dereference before null check")
> >
> > Fixes: 77e3e3b165db ("regulator: axp20x: add software based soft_start for 
> > AXP209 LDO3")
> > Signed-off-by: Colin Ian King 
> 
> This patch somehow fell through the cracks. Sorry about that.
> 
> Both are really false positives.

I don't like when people use the word "false positive" like this.  The
code doesn't checks for NULL after a dereference so that's a nonsense
to do.  The correct thing is to remove the NULL check.

regards,
dan carpenter

Re: [PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Chen-Yu Tsai]

On Sat, Dec 22, 2018 at 7:32 PM Colin King  wrote:
>
> From: Colin Ian King 
>
> Currently rdev is dereferenced when assigning desc before rdev is null
> checked, hence there is a potential null pointer dereference on rdev.
> Fix this by null checking rdev first.
>
> Detected by CoverityScan, CID#1476031 ("Dereference before null check")
>
> Fixes: 77e3e3b165db ("regulator: axp20x: add software based soft_start for 
> AXP209 LDO3")
> Signed-off-by: Colin Ian King 

This patch somehow fell through the cracks. Sorry about that.

Both are really false positives. Both functions are callbacks for the
regulator core to use. If you look through the core code, rdev->desc->ops
is used to check for these callbacks before they are called. rdev is always
passed in, and rdev->desc is always valid after regulator_register.

ChenYu


> ---
>
> V2: Fix 2nd occurrance of this thinko
>
> ---
>  drivers/regulator/axp20x-regulator.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/regulator/axp20x-regulator.c 
> b/drivers/regulator/axp20x-regulator.c
> index 48af859fd053..0dfa4ea6bbdf 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -367,7 +367,7 @@ static const int axp209_dcdc2_ldo3_slew_rates[] = {
>  static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
>  {
> struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> -   const struct regulator_desc *desc = rdev->desc;
> +   const struct regulator_desc *desc;
> u8 reg, mask, enable, cfg = 0xff;
> const int *slew_rates;
> int rate_count = 0;
> @@ -375,6 +375,8 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> *rdev, int ramp)
> if (!rdev)
> return -EINVAL;
>
> +   desc = rdev->desc;
> +
> switch (axp20x->variant) {
> case AXP209_ID:
> if (desc->id == AXP20X_DCDC2) {
> @@ -436,11 +438,13 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> *rdev, int ramp)
>  static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev)
>  {
> struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> -   const struct regulator_desc *desc = rdev->desc;
> +   const struct regulator_desc *desc;
>
> if (!rdev)
> return -EINVAL;
>
> +   desc = rdev->desc;
> +
> switch (axp20x->variant) {
> case AXP209_ID:
> if ((desc->id == AXP20X_LDO3) &&
> --
> 2.19.1
>

Re: [PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Dan Carpenter]

On Mon, Feb 04, 2019 at 07:13:30PM +, Colin Ian King wrote:
> >  drivers/regulator/axp20x-regulator.c | 8 ++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/regulator/axp20x-regulator.c 
> > b/drivers/regulator/axp20x-regulator.c
> > index 48af859fd053..0dfa4ea6bbdf 100644
> > --- a/drivers/regulator/axp20x-regulator.c
> > +++ b/drivers/regulator/axp20x-regulator.c
> > @@ -367,7 +367,7 @@ static const int axp209_dcdc2_ldo3_slew_rates[] = {
> >  static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
> >  {
> > struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> > -   const struct regulator_desc *desc = rdev->desc;
> > +   const struct regulator_desc *desc;

This is only called from set_machine_constraints() so "rdev" can't be
NULL.  (On my allmodonconfig build).

> > u8 reg, mask, enable, cfg = 0xff;
> > const int *slew_rates;
> > int rate_count = 0;
> > @@ -375,6 +375,8 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> > *rdev, int ramp)
> > if (!rdev)
> > return -EINVAL;
> >  
> > +   desc = rdev->desc;
> > +
> > switch (axp20x->variant) {
> > case AXP209_ID:
> > if (desc->id == AXP20X_DCDC2) {
> > @@ -436,11 +438,13 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> > *rdev, int ramp)
> >  static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev)
> >  {
> > struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> > -   const struct regulator_desc *desc = rdev->desc;
> > +   const struct regulator_desc *desc;
> >  
> > if (!rdev)
> > return -EINVAL;
> >  
> > +   desc = rdev->desc;

Called from _regulator_do_enable() and rdev can't be NULL:

ret = rdev->desc->ops->enable(rdev);

regards,
dan carpenter

Re: [PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Mark Brown]

On Mon, Feb 04, 2019 at 07:13:30PM +, Colin Ian King wrote:
> ping?

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  If there have been
review comments then people may be waiting for those to be addressed.

Sending content free pings adds to the mail volume (if they are seen at
all) which is often the problem and since they can't be reviewed
directly if something has gone wrong you'll have to resend the patches
anyway, so sending again is generally a better approach though there are
some other maintainers who like them - if in doubt look at how patches
for the subsystem are normally handled.


signature.asc
Description: PGP signature

Re: [PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Colin Ian King]

ping?

On 22/12/2018 11:31, Colin King wrote:
> From: Colin Ian King 
> 
> Currently rdev is dereferenced when assigning desc before rdev is null
> checked, hence there is a potential null pointer dereference on rdev.
> Fix this by null checking rdev first.
> 
> Detected by CoverityScan, CID#1476031 ("Dereference before null check")
> 
> Fixes: 77e3e3b165db ("regulator: axp20x: add software based soft_start for 
> AXP209 LDO3")
> Signed-off-by: Colin Ian King 
> ---
> 
> V2: Fix 2nd occurrance of this thinko
> 
> ---
>  drivers/regulator/axp20x-regulator.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/axp20x-regulator.c 
> b/drivers/regulator/axp20x-regulator.c
> index 48af859fd053..0dfa4ea6bbdf 100644
> --- a/drivers/regulator/axp20x-regulator.c
> +++ b/drivers/regulator/axp20x-regulator.c
> @@ -367,7 +367,7 @@ static const int axp209_dcdc2_ldo3_slew_rates[] = {
>  static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
>  {
>   struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> - const struct regulator_desc *desc = rdev->desc;
> + const struct regulator_desc *desc;
>   u8 reg, mask, enable, cfg = 0xff;
>   const int *slew_rates;
>   int rate_count = 0;
> @@ -375,6 +375,8 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> *rdev, int ramp)
>   if (!rdev)
>   return -EINVAL;
>  
> + desc = rdev->desc;
> +
>   switch (axp20x->variant) {
>   case AXP209_ID:
>   if (desc->id == AXP20X_DCDC2) {
> @@ -436,11 +438,13 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
> *rdev, int ramp)
>  static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev)
>  {
>   struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
> - const struct regulator_desc *desc = rdev->desc;
> + const struct regulator_desc *desc;
>  
>   if (!rdev)
>   return -EINVAL;
>  
> + desc = rdev->desc;
> +
>   switch (axp20x->variant) {
>   case AXP209_ID:
>   if ((desc->id == AXP20X_LDO3) &&
> 

[PATCH][V2][next] regulator: axp20x: check rdev is null before dereferencing it

[Colin King]

From: Colin Ian King 

Currently rdev is dereferenced when assigning desc before rdev is null
checked, hence there is a potential null pointer dereference on rdev.
Fix this by null checking rdev first.

Detected by CoverityScan, CID#1476031 ("Dereference before null check")

Fixes: 77e3e3b165db ("regulator: axp20x: add software based soft_start for 
AXP209 LDO3")
Signed-off-by: Colin Ian King 
---

V2: Fix 2nd occurrance of this thinko

---
 drivers/regulator/axp20x-regulator.c | 8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c 
b/drivers/regulator/axp20x-regulator.c
index 48af859fd053..0dfa4ea6bbdf 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -367,7 +367,7 @@ static const int axp209_dcdc2_ldo3_slew_rates[] = {
 static int axp20x_set_ramp_delay(struct regulator_dev *rdev, int ramp)
 {
struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
-   const struct regulator_desc *desc = rdev->desc;
+   const struct regulator_desc *desc;
u8 reg, mask, enable, cfg = 0xff;
const int *slew_rates;
int rate_count = 0;
@@ -375,6 +375,8 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
*rdev, int ramp)
if (!rdev)
return -EINVAL;
 
+   desc = rdev->desc;
+
switch (axp20x->variant) {
case AXP209_ID:
if (desc->id == AXP20X_DCDC2) {
@@ -436,11 +438,13 @@ static int axp20x_set_ramp_delay(struct regulator_dev 
*rdev, int ramp)
 static int axp20x_regulator_enable_regmap(struct regulator_dev *rdev)
 {
struct axp20x_dev *axp20x = rdev_get_drvdata(rdev);
-   const struct regulator_desc *desc = rdev->desc;
+   const struct regulator_desc *desc;
 
if (!rdev)
return -EINVAL;
 
+   desc = rdev->desc;
+
switch (axp20x->variant) {
case AXP209_ID:
if ((desc->id == AXP20X_LDO3) &&
-- 
2.19.1