[PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Chenyuan Yang]

lpc_snoop->chan[channel].miscdev.name could be NULL, thus,
a pointer check is added to prevent potential NULL pointer dereference.
This is similar to the fix in commit 3027e7b15b02
("ice: Fix some null pointer dereference issues in ice_ptp.c").

This issue is found by our static analysis tool.

Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
---
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 9ab5ba9cf1d6..376b3a910797 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -200,6 +200,8 @@ static int aspeed_lpc_enable_snoop(struct aspeed_lpc_snoop *lpc_snoop,
 	lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
 	lpc_snoop->chan[channel].miscdev.name =
 		devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
+	if (!lpc_snoop->chan[channel].miscdev.name)
+		return -ENOMEM;
 	lpc_snoop->chan[channel].miscdev.fops = &snoop_fops;
 	lpc_snoop->chan[channel].miscdev.parent = dev;
 	rc = misc_register(&lpc_snoop->chan[channel].miscdev);
-- 
2.34.1

Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Andrew Jeffery]

Hi Chenyuan,

On Wed, 2025-02-12 at 15:25 -0600, Chenyuan Yang wrote:
> lpc_snoop->chan[channel].miscdev.name could be NULL, thus,
> a pointer check is added to prevent potential NULL pointer
> dereference.
> This is similar to the fix in commit 3027e7b15b02
> ("ice: Fix some null pointer dereference issues in ice_ptp.c").
> 
> This issue is found by our static analysis tool.
> 
> Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> ---
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 9ab5ba9cf1d6..376b3a910797 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -200,6 +200,8 @@ static int aspeed_lpc_enable_snoop(struct
> aspeed_lpc_snoop *lpc_snoop,
>         lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
>         lpc_snoop->chan[channel].miscdev.name =
>                 devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> channel);
> +       if (!lpc_snoop->chan[channel].miscdev.name)
> +               return -ENOMEM;

This introduces yet another place where the driver leaks resources in
an error path (in this case, the channel kfifo). The misc device also
gets leaked later on. It would be nice to address those first so that
handling this error can take the appropriate cleanup path.

Andrew

Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Chenyuan Yang]

Hi Andrew,

Thanks for your prompt reply!

On Wed, Feb 12, 2025 at 6:21 PM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> Hi Chenyuan,
>
> On Wed, 2025-02-12 at 15:25 -0600, Chenyuan Yang wrote:
> > lpc_snoop->chan[channel].miscdev.name could be NULL, thus,
> > a pointer check is added to prevent potential NULL pointer
> > dereference.
> > This is similar to the fix in commit 3027e7b15b02
> > ("ice: FiI am cx some null pointer dereference issues in ice_ptp.c").
> >
> > This issue is found by our static analysis tool.
> >
> > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > ---
> >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > index 9ab5ba9cf1d6..376b3a910797 100644
> > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > @@ -200,6 +200,8 @@ static int aspeed_lpc_enable_snoop(struct
> > aspeed_lpc_snoop *lpc_snoop,
> >         lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
> >         lpc_snoop->chan[channel].miscdev.name =
> >                 devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> > channel);
> > +       if (!lpc_snoop->chan[channel].miscdev.name)
> > +               return -ENOMEM;
>
> This introduces yet another place where the driver leaks resources in
> an error path (in this case, the channel kfifo). The misc device also
> gets leaked later on. It would be nice to address those first so that
> handling this error can take the appropriate cleanup path.
>
> Andrew

It seems that the `aspeed_lpc_enable_snoop()` function originally does
not have a cleanup path. For example, if `misc_register` fails, the
function directly returns rc without performing any cleanup.
Similarly, when the `channel` has its default value, the function
simply returns -EINVAL.

Given this, I am wondering whether it would be a good idea to
introduce a cleanup path. If so, should we ensure cleanup for all
possible exit points?

Looking forward to your thoughts.

-Chenyuan

Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Andrew Jeffery]

On Wed, 2025-02-12 at 19:37 -0600, Chenyuan Yang wrote:
> Hi Andrew,
> 
> Thanks for your prompt reply!
> 
> On Wed, Feb 12, 2025 at 6:21 PM Andrew Jeffery
> <andrew@codeconstruct.com.au> wrote:
> > 
> > Hi Chenyuan,
> > 
> > On Wed, 2025-02-12 at 15:25 -0600, Chenyuan Yang wrote:
> > > lpc_snoop->chan[channel].miscdev.name could be NULL, thus,
> > > a pointer check is added to prevent potential NULL pointer
> > > dereference.
> > > This is similar to the fix in commit 3027e7b15b02
> > > ("ice: FiI am cx some null pointer dereference issues in ice_ptp.c").
> > > 
> > > This issue is found by our static analysis tool.
> > > 
> > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > > ---
> > >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > index 9ab5ba9cf1d6..376b3a910797 100644
> > > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > @@ -200,6 +200,8 @@ static int aspeed_lpc_enable_snoop(struct
> > > aspeed_lpc_snoop *lpc_snoop,
> > >         lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
> > >         lpc_snoop->chan[channel].miscdev.name =
> > >                 devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> > > channel);
> > > +       if (!lpc_snoop->chan[channel].miscdev.name)
> > > +               return -ENOMEM;
> > 
> > This introduces yet another place where the driver leaks resources in
> > an error path (in this case, the channel kfifo). The misc device also
> > gets leaked later on. It would be nice to address those first so that
> > handling this error can take the appropriate cleanup path.
> > 
> > Andrew
> 
> It seems that the `aspeed_lpc_enable_snoop()` function originally does
> not have a cleanup path. For example, if `misc_register` fails, the
> function directly returns rc without performing any cleanup.
> Similarly, when the `channel` has its default value, the function
> simply returns -EINVAL.
> 
> Given this, I am wondering whether it would be a good idea to
> introduce a cleanup path. If so, should we ensure cleanup for all
> possible exit points?

Yes please!

Thanks,

Andrew

Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Chenyuan Yang]

Hi Andrew,

I've drafted the following patch to address the resource cleanup issue:

```
 drivers/soc/aspeed/aspeed-lpc-snoop.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
b/drivers/soc/aspeed/aspeed-lpc-snoop.c
index 9ab5ba9cf1d6..4988144aba88 100644
--- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
+++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
@@ -200,11 +200,15 @@ static int aspeed_lpc_enable_snoop(struct
aspeed_lpc_snoop *lpc_snoop,
  lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
  lpc_snoop->chan[channel].miscdev.name =
  devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
+ if (!lpc_snoop->chan[channel].miscdev.name) {
+ rc = -ENOMEM;
+ goto free_fifo;
+ }
  lpc_snoop->chan[channel].miscdev.fops = &snoop_fops;
  lpc_snoop->chan[channel].miscdev.parent = dev;
  rc = misc_register(&lpc_snoop->chan[channel].miscdev);
  if (rc)
- return rc;
+ goto free_name;

  /* Enable LPC snoop channel at requested port */
  switch (channel) {
@@ -221,7 +225,8 @@ static int aspeed_lpc_enable_snoop(struct
aspeed_lpc_snoop *lpc_snoop,
  hicrb_en = HICRB_ENSNP1D;
  break;
  default:
- return -EINVAL;
+ rc = -EINVAL;
+ goto free_misc;
  }

  regmap_update_bits(lpc_snoop->regmap, HICR5, hicr5_en, hicr5_en);
@@ -232,6 +237,14 @@ static int aspeed_lpc_enable_snoop(struct
aspeed_lpc_snoop *lpc_snoop,
  hicrb_en, hicrb_en);

  return rc;
+
+free_misc:
+ misc_deregister(&lpc_snoop->chan[channel].miscdev);
+free_name:
+ kfree(lpc_snoop->chan[channel].miscdev.name);
+free_fifo:
+ kfifo_free(&lpc_snoop->chan[channel].fifo);
+ return rc;
 }
```

I have a couple of questions regarding the cleanup order:

1. Do we need to call misc_deregister() in this case, considering that
the registration happens before return -EINVAL?
2. If misc_deregister() is required, is there a specific order we
should follow when calling misc_deregister() and
kfree(lpc_snoop->chan[channel].miscdev.name);?

-Chenyuan

On Wed, Feb 12, 2025 at 7:39 PM Andrew Jeffery
<andrew@codeconstruct.com.au> wrote:
>
> On Wed, 2025-02-12 at 19:37 -0600, Chenyuan Yang wrote:
> > Hi Andrew,
> >
> > Thanks for your prompt reply!
> >
> > On Wed, Feb 12, 2025 at 6:21 PM Andrew Jeffery
> > <andrew@codeconstruct.com.au> wrote:
> > >
> > > Hi Chenyuan,
> > >
> > > On Wed, 2025-02-12 at 15:25 -0600, Chenyuan Yang wrote:
> > > > lpc_snoop->chan[channel].miscdev.name could be NULL, thus,
> > > > a pointer check is added to prevent potential NULL pointer
> > > > dereference.
> > > > This is similar to the fix in commit 3027e7b15b02
> > > > ("ice: FiI am cx some null pointer dereference issues in ice_ptp.c").
> > > >
> > > > This issue is found by our static analysis tool.
> > > >
> > > > Signed-off-by: Chenyuan Yang <chenyuan0y@gmail.com>
> > > > ---
> > > >  drivers/soc/aspeed/aspeed-lpc-snoop.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > > b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > > index 9ab5ba9cf1d6..376b3a910797 100644
> > > > --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > > +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> > > > @@ -200,6 +200,8 @@ static int aspeed_lpc_enable_snoop(struct
> > > > aspeed_lpc_snoop *lpc_snoop,
> > > >         lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
> > > >         lpc_snoop->chan[channel].miscdev.name =
> > > >                 devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME,
> > > > channel);
> > > > +       if (!lpc_snoop->chan[channel].miscdev.name)
> > > > +               return -ENOMEM;
> > >
> > > This introduces yet another place where the driver leaks resources in
> > > an error path (in this case, the channel kfifo). The misc device also
> > > gets leaked later on. It would be nice to address those first so that
> > > handling this error can take the appropriate cleanup path.
> > >
> > > Andrew
> >
> > It seems that the `aspeed_lpc_enable_snoop()` function originally does
> > not have a cleanup path. For example, if `misc_register` fails, the
> > function directly returns rc without performing any cleanup.
> > Similarly, when the `channel` has its default value, the function
> > simply returns -EINVAL.
> >
> > Given this, I am wondering whether it would be a good idea to
> > introduce a cleanup path. If so, should we ensure cleanup for all
> > possible exit points?
>
> Yes please!
>
> Thanks,
>
> Andrew
>

Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()

[Andrew Lunn]

On Wed, Feb 12, 2025 at 07:50:49PM -0600, Chenyuan Yang wrote:
> Hi Andrew,
> 
> I've drafted the following patch to address the resource cleanup issue:

Please just follow the normal procedure of submitting a patch.

	Andrew

> 
> ```
>  drivers/soc/aspeed/aspeed-lpc-snoop.c | 17 +++++++++++++++--
>  1 file changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> index 9ab5ba9cf1d6..4988144aba88 100644
> --- a/drivers/soc/aspeed/aspeed-lpc-snoop.c
> +++ b/drivers/soc/aspeed/aspeed-lpc-snoop.c
> @@ -200,11 +200,15 @@ static int aspeed_lpc_enable_snoop(struct
> aspeed_lpc_snoop *lpc_snoop,
>   lpc_snoop->chan[channel].miscdev.minor = MISC_DYNAMIC_MINOR;
>   lpc_snoop->chan[channel].miscdev.name =
>   devm_kasprintf(dev, GFP_KERNEL, "%s%d", DEVICE_NAME, channel);
> + if (!lpc_snoop->chan[channel].miscdev.name) {
> + rc = -ENOMEM;
> + goto free_fifo;
> + }

You were asked to first add cleanup, then fix this possible NULL
pointer dereference.

> I have a couple of questions regarding the cleanup order:
> 
> 1. Do we need to call misc_deregister() in this case, considering that
> the registration happens before return -EINVAL?
> 2. If misc_deregister() is required, is there a specific order we
> should follow when calling misc_deregister() and
> kfree(lpc_snoop->chan[channel].miscdev.name);?

As a general rule, cleanup is the opposite order to setup.

Also, you want to do some research about that devm_ means.

	Andrew