| Message ID | 20250212212556.2667-1-chenyuan0y@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop() | expand |
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
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
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
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
>
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
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);
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(+)