* [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path
@ 2021-10-26 18:07 Vincent Mailhol
2021-10-27 7:39 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Vincent Mailhol @ 2021-10-26 18:07 UTC (permalink / raw)
To: Marc Kleine-Budde, linux-can; +Cc: netdev, linux-kernel, Vincent Mailhol
In es58x_rx_err_msg(), if can->do_set_mode() fails, the function
directly returns without calling netif_rx(skb). This means that the
skb previously allocated by alloc_can_err_skb() is not freed. In other
terms, this is a memory leak.
This patch simply removes the return statement in the error branch and
let the function continue.
* Appendix: how the issue was found *
This issue was found using GCC's static analysis tool: -fanalyzer:
https://gcc.gnu.org/onlinedocs/gcc/Static-Analyzer-Options.html
The step to reproduce are:
1. Install GCC 11.
2. Hack the kernel's Makefile to add the -fanalyzer flag (we leave
it as an exercise for the reader to figure out the details of how to
do so).
3. Decorate the function alloc_can_err_skb() with
__attribute__((__malloc__ (dealloc, netif_rx))). This step helps the
static analyzer to figure out the constructor/destructor pairs (not
something it can deduce by himself).
4. Compile.
The compiler then throws below warning:
| In function 'es58x_rx_err_msg':
| drivers/net/can/usb/etas_es58x/es58x_core.c:826:28: warning: leak of 'skb' [CWE-401] [-Wanalyzer-malloc-leak]
| 826 | if (ret)
| | ^
| 'es58x_rx_err_msg': events 1-9
| |
| | 659 | int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
| | | ^~~~~~~~~~~~~~~~
| | | |
| | | (1) entry to 'es58x_rx_err_msg'
| |......
| | 669 | if (!netif_running(netdev)) {
| | | ~
| | | |
| | | (2) following 'true' branch...
| |......
| | 677 | if (error == ES58X_ERR_OK && event == ES58X_EVENT_OK) {
| | | ~~ ~
| | | | |
| | | | (4) following 'false' branch...
| | | (3) ...to here
| |......
| | 683 | skb = alloc_can_err_skb(netdev, &cf);
| | | ~~~
| | | |
| | | (5) ...to here
| |......
| | 861 | if (cf) {
| | | ~
| | | |
| | | (6) following 'false' branch...
| |......
| | 875 | if ((event & ES58X_EVENT_CRTL_PASSIVE) &&
| | | ~~ ~
| | | | |
| | | | (8) following 'true' branch...
| | | (7) ...to here
| | 876 | priv->err_passive_before_rtx_success == ES58X_CONSECUTIVE_ERR_PASSIVE_MAX) {
| | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | |
| | | (9) ...to here
| |
| 'es58x_rx_err_msg': events 10-12
| |
| | 875 | if ((event & ES58X_EVENT_CRTL_PASSIVE) &&
| | 876 | priv->err_passive_before_rtx_success == ES58X_CONSECUTIVE_ERR_PASSIVE_MAX) {
| | 877 | netdev_info(netdev,
| | | ~~~~~~~~~~~
| | | |
| | | (11) ...to here
| |......
| | 880 | return es58x_rx_err_msg(netdev, ES58X_ERR_OK,
| | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | |
| | | (12) calling 'es58x_rx_err_msg' from 'es58x_rx_err_msg'
| | 881 | ES58X_EVENT_BUSOFF, timestamp);
| | | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| +--> 'es58x_rx_err_msg': events 13-23
| |
| | 659 | int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
| | | ^~~~~~~~~~~~~~~~
| | | |
| | | (13) entry to 'es58x_rx_err_msg'
| |......
| | 669 | if (!netif_running(netdev)) {
| | | ~
| | | |
| | | (14) following 'true' branch...
| |......
| | 677 | if (error == ES58X_ERR_OK && event == ES58X_EVENT_OK) {
| | | ~~ ~
| | | | |
| | | | (16) following 'false' branch...
| | | (15) ...to here
| |......
| | 683 | skb = alloc_can_err_skb(netdev, &cf);
| | | ~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | | | |
| | | | (18) allocated here
| | | (17) ...to here
| | 684 |
| | 685 | switch (error) {
| | | ~~~~~~
| | | |
| | | (19) following 'case 0:' branch...
| |......
| | 764 | switch (event) {
| | | ~~~~~~
| | | |
| | | (20) ...to here
| | | (21) following 'case 8:' branch...
| |......
| | 815 | case ES58X_EVENT_BUSOFF:
| | | ~~~~
| | | |
| | | (22) ...to here
| |......
| | 826 | if (ret)
| | | ~
| | | |
| | | (23) 'skb' leaks here; was allocated at (18)
| |
Fixes: 8537257874e9 ("can: etas_es58x: add core support for ETAS ES58X CAN USB interfaces")
Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
drivers/net/can/usb/etas_es58x/es58x_core.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/net/can/usb/etas_es58x/es58x_core.c b/drivers/net/can/usb/etas_es58x/es58x_core.c
index 403de7e9d084..8508a73d648e 100644
--- a/drivers/net/can/usb/etas_es58x/es58x_core.c
+++ b/drivers/net/can/usb/etas_es58x/es58x_core.c
@@ -664,7 +664,7 @@ int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
struct can_device_stats *can_stats = &can->can_stats;
struct can_frame *cf = NULL;
struct sk_buff *skb;
- int ret;
+ int ret = 0;
if (!netif_running(netdev)) {
if (net_ratelimit())
@@ -823,8 +823,6 @@ int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
can->state = CAN_STATE_BUS_OFF;
can_bus_off(netdev);
ret = can->do_set_mode(netdev, CAN_MODE_STOP);
- if (ret)
- return ret;
}
break;
@@ -881,7 +879,7 @@ int es58x_rx_err_msg(struct net_device *netdev, enum es58x_err error,
ES58X_EVENT_BUSOFF, timestamp);
}
- return 0;
+ return ret;
}
/**
--
2.32.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path
2021-10-26 18:07 [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path Vincent Mailhol
@ 2021-10-27 7:39 ` Marc Kleine-Budde
2021-10-27 8:48 ` Vincent MAILHOL
0 siblings, 1 reply; 4+ messages in thread
From: Marc Kleine-Budde @ 2021-10-27 7:39 UTC (permalink / raw)
To: Vincent Mailhol; +Cc: linux-can, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 820 bytes --]
On 27.10.2021 03:07:40, Vincent Mailhol wrote:
> In es58x_rx_err_msg(), if can->do_set_mode() fails, the function
> directly returns without calling netif_rx(skb). This means that the
> skb previously allocated by alloc_can_err_skb() is not freed. In other
> terms, this is a memory leak.
>
> This patch simply removes the return statement in the error branch and
> let the function continue.
>
> * Appendix: how the issue was found *
Thanks for the explanation, but I think I'll remove the appendix while
applying.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path
2021-10-27 7:39 ` Marc Kleine-Budde
@ 2021-10-27 8:48 ` Vincent MAILHOL
2021-10-29 11:20 ` Marc Kleine-Budde
0 siblings, 1 reply; 4+ messages in thread
From: Vincent MAILHOL @ 2021-10-27 8:48 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: linux-can, netdev, open list
On Wed. 27 Oct 2021 at 16:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> On 27.10.2021 03:07:40, Vincent Mailhol wrote:
> > In es58x_rx_err_msg(), if can->do_set_mode() fails, the function
> > directly returns without calling netif_rx(skb). This means that the
> > skb previously allocated by alloc_can_err_skb() is not freed. In other
> > terms, this is a memory leak.
> >
> > This patch simply removes the return statement in the error branch and
> > let the function continue.
> >
> > * Appendix: how the issue was found *
>
> Thanks for the explanation, but I think I'll remove the appendix while
> applying.
The commit will have a link to this thread so if you don't mind,
I suggest to replace the full appendix with:
Issue was found with GCC -fanalyzer, please follow the link below
for details.
You can of course do the same for the m_can patch.
Yours sincerely,
Vincent Mailhol
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path
2021-10-27 8:48 ` Vincent MAILHOL
@ 2021-10-29 11:20 ` Marc Kleine-Budde
0 siblings, 0 replies; 4+ messages in thread
From: Marc Kleine-Budde @ 2021-10-29 11:20 UTC (permalink / raw)
To: Vincent MAILHOL; +Cc: linux-can, netdev, open list
[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]
On 27.10.2021 17:48:21, Vincent MAILHOL wrote:
> On Wed. 27 Oct 2021 at 16:39, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 27.10.2021 03:07:40, Vincent Mailhol wrote:
> > > In es58x_rx_err_msg(), if can->do_set_mode() fails, the function
> > > directly returns without calling netif_rx(skb). This means that the
> > > skb previously allocated by alloc_can_err_skb() is not freed. In other
> > > terms, this is a memory leak.
> > >
> > > This patch simply removes the return statement in the error branch and
> > > let the function continue.
> > >
> > > * Appendix: how the issue was found *
> >
> > Thanks for the explanation, but I think I'll remove the appendix while
> > applying.
>
> The commit will have a link to this thread so if you don't mind,
> I suggest to replace the full appendix with:
>
> Issue was found with GCC -fanalyzer, please follow the link below
> for details.
Makes sense, good idea.
> You can of course do the same for the m_can patch.
ACK
Added to linux-can/testing, will send it out once v5.15 is out.
Thanks,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung West/Dortmund | Phone: +49-231-2826-924 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-29 11:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-26 18:07 [PATCH v1] can: etas_es58x: es58x_rx_err_msg: fix memory leak in error path Vincent Mailhol
2021-10-27 7:39 ` Marc Kleine-Budde
2021-10-27 8:48 ` Vincent MAILHOL
2021-10-29 11:20 ` Marc Kleine-Budde
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.