| Message ID | 20220708175949.539064-1-pchelkin@ispras.ru (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | can: j1939: fix memory leak of skbs | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Fedor, thank you for work. On Fri, Jul 08, 2022 at 08:59:49PM +0300, Fedor Pchelkin wrote: > 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); > } This skb_get() is counter part of skb_unref() j1939_session_skb_drop_old(). Initial issue can be reproduced by using real (slow) CAN with j1939cat[1] tool. Both parts should be started to make sure the j1939_session_tx_dat() will actually start using the queue. After pushing about 100K of data, application will try to close the socket and exit. After socket is closed, all skb related to this socket will be freed and j1939_session_tx_dat() will use freed skbs. NACK for this patch. 1. https://github.com/linux-can/can-utils/blob/master/j1939cat.c
Hi Oleksij, On 29.07.2022 07:22, Oleksij Rempel wrote: > Initial issue can be reproduced by using real (slow) CAN with j1939cat[1] > tool. Both parts should be started to make sure the j1939_session_tx_dat() will > actually start using the queue. After pushing about 100K of data, application > will try to close the socket and exit. After socket is closed, all skb related > to this socket will be freed and j1939_session_tx_dat() will use freed skbs. Ok, the patch I suggested was a kind of a guess, now I understand that it breaks important logic. On 29.07.2022 07:22, Oleksij Rempel wrote: > This skb_get() is counter part of skb_unref() > j1939_session_skb_drop_old(). However, we have a case [1] where j1939_session_skb_queue() is called but the corresponding j1939_session_skb_drop_old() is not called and it causes a memory leak. I tried to investigate it a little bit: the thing is that j1939_session_skb_drop_old() can be called only when processing J1939_ETP_CMD_CTS. On the contrary, as I can see, j1939_session_skb_queue() can be called independently from J1939_ETP_CMD_CTS so the two functions obviously do not correspond to each other. In reproducer case there is a J1939_ETP_CMD_RTS processing, then we send some messages (where j1939_session_skb_queue() is called) and after that J1939_ETP_CMD_ABORT is processed and we lose those skbs. [1] https://forge.ispras.ru/issues/11743
On Fri, Aug 05, 2022 at 11:56:18AM +0300, Fedor Pchelkin wrote: > Hi Oleksij, > > On 29.07.2022 07:22, Oleksij Rempel wrote: > > Initial issue can be reproduced by using real (slow) CAN with j1939cat[1] > > tool. Both parts should be started to make sure the j1939_session_tx_dat() will > > actually start using the queue. After pushing about 100K of data, application > > will try to close the socket and exit. After socket is closed, all skb related > > to this socket will be freed and j1939_session_tx_dat() will use freed skbs. > > Ok, the patch I suggested was a kind of a guess, now I understand that > it breaks important logic. > > On 29.07.2022 07:22, Oleksij Rempel wrote: > > This skb_get() is counter part of skb_unref() > > j1939_session_skb_drop_old(). > > However, we have a case [1] where j1939_session_skb_queue() is called > but the corresponding j1939_session_skb_drop_old() is not called and it > causes a memory leak. > > I tried to investigate it a little bit: the thing is that > j1939_session_skb_drop_old() can be called only when processing > J1939_ETP_CMD_CTS. On the contrary, as I can see, > j1939_session_skb_queue() can be called independently from > J1939_ETP_CMD_CTS so the two functions obviously do not correspond to > each other. > > In reproducer case there is a J1939_ETP_CMD_RTS processing, then > we send some messages (where j1939_session_skb_queue() is called) and > after that J1939_ETP_CMD_ABORT is processed and we lose those skbs. Ah.. good point. In this case it will go to: j1939_session_destroy() skb_queue_purge(&session->skb_queue) kfree_skb(skb); And in the normal path we have: j1939_session_skb_drop_old() skb_unref(do_skb); kfree_skb(do_skb); It means skb_queue_purge() should be replaced with something like: while ((skb = skb_dequeue(list)) != NULL) { /* drop ref taken in j1939_session_skb_queue() */ skb_unref(skb); kfree_skb(skb); } Can you please test it? Regards, Oleksij
On 05.08.2022 12:55, Oleksij Rempel wrote:
> Can you please test it?
That works fine. I'm preparing a patch for the issue.
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); }
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(-)