* [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
@ 2025-02-12 21:25 Chenyuan Yang
2025-02-13 0:20 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: Chenyuan Yang @ 2025-02-12 21:25 UTC (permalink / raw)
To: joel, andrew, richardcochran
Cc: linux-arm-kernel, linux-aspeed, linux-kernel, netdev, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
2025-02-12 21:25 [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop() Chenyuan Yang
@ 2025-02-13 0:20 ` Andrew Jeffery
2025-02-13 1:37 ` Chenyuan Yang
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2025-02-13 0:20 UTC (permalink / raw)
To: Chenyuan Yang, joel, richardcochran
Cc: linux-arm-kernel, linux-aspeed, linux-kernel, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
2025-02-13 0:20 ` Andrew Jeffery
@ 2025-02-13 1:37 ` Chenyuan Yang
2025-02-13 1:39 ` Andrew Jeffery
0 siblings, 1 reply; 6+ messages in thread
From: Chenyuan Yang @ 2025-02-13 1:37 UTC (permalink / raw)
To: Andrew Jeffery
Cc: joel, richardcochran, linux-arm-kernel, linux-aspeed,
linux-kernel, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
2025-02-13 1:37 ` Chenyuan Yang
@ 2025-02-13 1:39 ` Andrew Jeffery
2025-02-13 1:50 ` Chenyuan Yang
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Jeffery @ 2025-02-13 1:39 UTC (permalink / raw)
To: Chenyuan Yang
Cc: joel, richardcochran, linux-arm-kernel, linux-aspeed,
linux-kernel, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
2025-02-13 1:39 ` Andrew Jeffery
@ 2025-02-13 1:50 ` Chenyuan Yang
2025-02-13 13:55 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Chenyuan Yang @ 2025-02-13 1:50 UTC (permalink / raw)
To: Andrew Jeffery
Cc: joel, richardcochran, linux-arm-kernel, linux-aspeed,
linux-kernel, netdev
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop()
2025-02-13 1:50 ` Chenyuan Yang
@ 2025-02-13 13:55 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2025-02-13 13:55 UTC (permalink / raw)
To: Chenyuan Yang
Cc: Andrew Jeffery, joel, richardcochran, linux-arm-kernel,
linux-aspeed, linux-kernel, netdev
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-02-13 13:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-12 21:25 [PATCH] soc: aspeed: Add NULL pointer check in aspeed_lpc_enable_snoop() Chenyuan Yang
2025-02-13 0:20 ` Andrew Jeffery
2025-02-13 1:37 ` Chenyuan Yang
2025-02-13 1:39 ` Andrew Jeffery
2025-02-13 1:50 ` Chenyuan Yang
2025-02-13 13:55 ` Andrew Lunn
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®