diff mbox series

power: sequencing: fix NULL-pointer dereference in error path

Message ID 20240712144546.222119-1-brgl@bgdev.pl
State New
Headers show
Series power: sequencing: fix NULL-pointer dereference in error path | expand

Commit Message

Bartosz Golaszewski July 12, 2024, 2:45 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

We may call pwrseq_target_free() on a target without the final unit
assigned yet. In this case pwrseq_unit_put() will dereference
a NULL-pointer. Add a check to the latter function.

Fixes: 249ebf3f65f8 ("power: sequencing: implement the pwrseq core")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-pm/62a3531e-9927-40f8-b587-254a2dfa47ef@stanley.mountain/
Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/power/sequencing/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Dan Carpenter July 12, 2024, 2:59 p.m. UTC | #1 
On Fri, Jul 12, 2024 at 04:45:46PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> We may call pwrseq_target_free() on a target without the final unit
> assigned yet. In this case pwrseq_unit_put() will dereference
> a NULL-pointer. Add a check to the latter function.
> 
> Fixes: 249ebf3f65f8 ("power: sequencing: implement the pwrseq core")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-pm/62a3531e-9927-40f8-b587-254a2dfa47ef@stanley.mountain/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/power/sequencing/core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> index 9c32b07a55e7..fe07100e4b33 100644
> --- a/drivers/power/sequencing/core.c
> +++ b/drivers/power/sequencing/core.c
> @@ -119,7 +119,8 @@ static void pwrseq_unit_release(struct kref *ref);
>  
>  static void pwrseq_unit_put(struct pwrseq_unit *unit)
>  {
> -	kref_put(&unit->ref, pwrseq_unit_release);
> +	if (unit)

I was wondering where you would put the check.  But it needs to be:

	if (!IS_ERR_OR_NULL(unit))

regards,
dan carpenter

> +		kref_put(&unit->ref, pwrseq_unit_release);
>  }
>  
>  /**
> -- 
> 2.43.0
Bartosz Golaszewski July 12, 2024, 3:02 p.m. UTC | #2 
On Fri, Jul 12, 2024 at 4:59 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Jul 12, 2024 at 04:45:46PM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > We may call pwrseq_target_free() on a target without the final unit
> > assigned yet. In this case pwrseq_unit_put() will dereference
> > a NULL-pointer. Add a check to the latter function.
> >
> > Fixes: 249ebf3f65f8 ("power: sequencing: implement the pwrseq core")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/linux-pm/62a3531e-9927-40f8-b587-254a2dfa47ef@stanley.mountain/
> > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > ---
> >  drivers/power/sequencing/core.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> > index 9c32b07a55e7..fe07100e4b33 100644
> > --- a/drivers/power/sequencing/core.c
> > +++ b/drivers/power/sequencing/core.c
> > @@ -119,7 +119,8 @@ static void pwrseq_unit_release(struct kref *ref);
> >
> >  static void pwrseq_unit_put(struct pwrseq_unit *unit)
> >  {
> > -     kref_put(&unit->ref, pwrseq_unit_release);
> > +     if (unit)
>
> I was wondering where you would put the check.  But it needs to be:
>
>         if (!IS_ERR_OR_NULL(unit))
>
> regards,
> dan carpenter
>

Am I missing something? pwrseq_unit_new() can only return NULL on error.

Bart

> > +             kref_put(&unit->ref, pwrseq_unit_release);
> >  }
> >
> >  /**
> > --
> > 2.43.0
Dan Carpenter July 12, 2024, 3:24 p.m. UTC | #3 
On Fri, Jul 12, 2024 at 05:02:25PM +0200, Bartosz Golaszewski wrote:
> On Fri, Jul 12, 2024 at 4:59 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >
> > On Fri, Jul 12, 2024 at 04:45:46PM +0200, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > We may call pwrseq_target_free() on a target without the final unit
> > > assigned yet. In this case pwrseq_unit_put() will dereference
> > > a NULL-pointer. Add a check to the latter function.
> > >
> > > Fixes: 249ebf3f65f8 ("power: sequencing: implement the pwrseq core")
> > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > Closes: https://lore.kernel.org/linux-pm/62a3531e-9927-40f8-b587-254a2dfa47ef@stanley.mountain/
> > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > ---
> > >  drivers/power/sequencing/core.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> > > index 9c32b07a55e7..fe07100e4b33 100644
> > > --- a/drivers/power/sequencing/core.c
> > > +++ b/drivers/power/sequencing/core.c
> > > @@ -119,7 +119,8 @@ static void pwrseq_unit_release(struct kref *ref);
> > >
> > >  static void pwrseq_unit_put(struct pwrseq_unit *unit)
> > >  {
> > > -     kref_put(&unit->ref, pwrseq_unit_release);
> > > +     if (unit)
> >
> > I was wondering where you would put the check.  But it needs to be:
> >
> >         if (!IS_ERR_OR_NULL(unit))
> >
> > regards,
> > dan carpenter
> >
> 
> Am I missing something? pwrseq_unit_new() can only return NULL on error.
> 

It's not pwrseq_unit_new() but pwrseq_unit_setup() that's the issue.
The target->unit = pwrseq_unit_setup() assignment in
pwrseq_do_setup_targets().

regards,
dan carpenter
Bartosz Golaszewski July 12, 2024, 6:23 p.m. UTC | #4 
On Fri, Jul 12, 2024 at 5:24 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Fri, Jul 12, 2024 at 05:02:25PM +0200, Bartosz Golaszewski wrote:
> > On Fri, Jul 12, 2024 at 4:59 PM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> > >
> > > On Fri, Jul 12, 2024 at 04:45:46PM +0200, Bartosz Golaszewski wrote:
> > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > >
> > > > We may call pwrseq_target_free() on a target without the final unit
> > > > assigned yet. In this case pwrseq_unit_put() will dereference
> > > > a NULL-pointer. Add a check to the latter function.
> > > >
> > > > Fixes: 249ebf3f65f8 ("power: sequencing: implement the pwrseq core")
> > > > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > > > Closes: https://lore.kernel.org/linux-pm/62a3531e-9927-40f8-b587-254a2dfa47ef@stanley.mountain/
> > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > > > ---
> > > >  drivers/power/sequencing/core.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> > > > index 9c32b07a55e7..fe07100e4b33 100644
> > > > --- a/drivers/power/sequencing/core.c
> > > > +++ b/drivers/power/sequencing/core.c
> > > > @@ -119,7 +119,8 @@ static void pwrseq_unit_release(struct kref *ref);
> > > >
> > > >  static void pwrseq_unit_put(struct pwrseq_unit *unit)
> > > >  {
> > > > -     kref_put(&unit->ref, pwrseq_unit_release);
> > > > +     if (unit)
> > >
> > > I was wondering where you would put the check.  But it needs to be:
> > >
> > >         if (!IS_ERR_OR_NULL(unit))
> > >
> > > regards,
> > > dan carpenter
> > >
> >
> > Am I missing something? pwrseq_unit_new() can only return NULL on error.
> >
>
> It's not pwrseq_unit_new() but pwrseq_unit_setup() that's the issue.
> The target->unit = pwrseq_unit_setup() assignment in
> pwrseq_do_setup_targets().
>
> regards,
> dan carpenter
>

Indeed. Thanks.

Bart
diff mbox series

Patch

diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
index 9c32b07a55e7..fe07100e4b33 100644
--- a/drivers/power/sequencing/core.c
+++ b/drivers/power/sequencing/core.c
@@ -119,7 +119,8 @@  static void pwrseq_unit_release(struct kref *ref);
 
 static void pwrseq_unit_put(struct pwrseq_unit *unit)
 {
-	kref_put(&unit->ref, pwrseq_unit_release);
+	if (unit)
+		kref_put(&unit->ref, pwrseq_unit_release);
 }
 
 /**