All of lore.kernel.org
 help / color / mirror / Atom feed
From: Fedor Pchelkin <pchelkin@ispras.ru>
To: Robin van der Gracht <robin@protonic.nl>,
	Oleksij Rempel <linux@rempel-privat.de>
Cc: Fedor Pchelkin <pchelkin@ispras.ru>,
	kernel@pengutronix.de, Oliver Hartkopp <socketcan@hartkopp.net>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-can@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, ldv-project@linuxtesting.org,
	Alexey Khoroshilov <khoroshilov@ispras.ru>
Subject: [PATCH] can: j1939: fix memory leak of skbs
Date: Fri,  8 Jul 2022 20:59:49 +0300	[thread overview]
Message-ID: <20220708175949.539064-1-pchelkin@ispras.ru> (raw)

Syzkaller reported memory leak of skbs introduced with the commit
2030043e616c ("can: j1939: fix Use-after-Free, hold skb ref while in use").

Link to Syzkaller info and repro: https://forge.ispras.ru/issues/11743

The suggested solution was tested on the new memory-leak Syzkaller repro
and on the old use-after-free repro (that use-after-free bug was solved
with aforementioned commit). Although there can probably be another
situations when the numbers of skb_get() and skb_unref() calls don't match
and I don't see it in right way.

Moreover, skb_unref() call can be harmlessly removed from line 338 in
j1939_session_skb_drop_old() (/net/can/j1939/transport.c). But then I
assume this removal ruins the whole reference counts logic...

Overall, there is definitely something not clear in skb reference counts
management with skb_get() and skb_unref(). The solution we suggested fixes
the leaks and use-after-free's induced by Syzkaller but perhaps the origin
of the problem can be somewhere else.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
 net/can/j1939/transport.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/can/j1939/transport.c b/net/can/j1939/transport.c
index 307ee1174a6e..9600b339cbf8 100644
--- a/net/can/j1939/transport.c
+++ b/net/can/j1939/transport.c
@@ -356,7 +356,6 @@ void j1939_session_skb_queue(struct j1939_session *session,
 
 	skcb->flags |= J1939_ECU_LOCAL_SRC;
 
-	skb_get(skb);
 	skb_queue_tail(&session->skb_queue, skb);
 }
 
-- 
2.25.1


             reply	other threads:[~2022-07-08 18:00 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-08 17:59 Fedor Pchelkin [this message]
2022-07-29  4:22 ` [PATCH] can: j1939: fix memory leak of skbs Oleksij Rempel
2022-08-05  8:56   ` Fedor Pchelkin
2022-08-05  9:55     ` Oleksij Rempel
2022-08-05 14:55       ` Fedor Pchelkin
2022-08-05 15:02 Fedor Pchelkin
2022-08-05 16:20 ` Oleksij Rempel
2022-08-08  7:56 ` Marc Kleine-Budde

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220708175949.539064-1-pchelkin@ispras.ru \
    --to=pchelkin@ispras.ru \
    --cc=davem@davemloft.net \
    --cc=edumazet@google.com \
    --cc=kernel@pengutronix.de \
    --cc=khoroshilov@ispras.ru \
    --cc=kuba@kernel.org \
    --cc=ldv-project@linuxtesting.org \
    --cc=linux-can@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@rempel-privat.de \
    --cc=mkl@pengutronix.de \
    --cc=netdev@vger.kernel.org \
    --cc=pabeni@redhat.com \
    --cc=robin@protonic.nl \
    --cc=socketcan@hartkopp.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.