| Message ID | 20240205170117.250866-1-dmantipov@yandex.ru (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: sctp: fix memory leak in sctp_chunk_destroy() | expand |
On Mon, Feb 5, 2024 at 12:02 PM Dmitry Antipov <dmantipov@yandex.ru> wrote: > > In case of GSO, per-chunk 'skb' pointer may point to an entry from > fraglist created in 'sctp_packet_gso_append()'. To avoid freeing > random fraglist entry (and so undefined behavior and/or memory > leak), consume 'head_skb' (i.e. beginning of a fraglist) instead. Right, chunk->skb is supposed to be set to chunk->head_skb before calling sctp_chunk_free () in sctp_inq_pop(): if (chunk->head_skb) chunk->skb = chunk->head_skb; sctp_chunk_free(chunk); chunk = queue->in_progress = NULL; However, somehow the loop in sctp_assoc_bh_rcv() breaks without dequeuing all skbs, and I guess it's because of "asoc->base.dead". In this case, sctp_inq_free() should take care of its release, so can you try to fix it there? like: diff --git a/net/sctp/inqueue.c b/net/sctp/inqueue.c index 7182c5a450fb..dda5e1ad9cac 100644 --- a/net/sctp/inqueue.c +++ b/net/sctp/inqueue.c @@ -52,8 +52,11 @@ void sctp_inq_free(struct sctp_inq *queue) /* If there is a packet which is currently being worked on, * free it as well. */ - if (queue->in_progress) { - sctp_chunk_free(queue->in_progress); + chunk = queue->in_progress; + if (chunk) { + if (chunk->head_skb) + chunk->skb = chunk->head_skb; + sctp_chunk_free(chunk); queue->in_progress = NULL; } } this would avoid doing chunk->head_skb check for all chunks destroying. Thanks. > > Reported-by: syzbot+8bb053b5d63595ab47db@syzkaller.appspotmail.com > Closes: https://syzkaller.appspot.com/bug?id=0d8351bbe54fd04a492c2daab0164138db008042 > Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> > --- > net/sctp/sm_make_chunk.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c > index f80208edd6a5..30fe34743009 100644 > --- a/net/sctp/sm_make_chunk.c > +++ b/net/sctp/sm_make_chunk.c > @@ -1500,7 +1500,10 @@ static void sctp_chunk_destroy(struct sctp_chunk *chunk) > BUG_ON(!list_empty(&chunk->list)); > list_del_init(&chunk->transmitted_list); > > - consume_skb(chunk->skb); > + /* In case of GSO, 'skb' may be a pointer to fraglist entry. > + * Consume the read head if so. > + */ > + consume_skb(chunk->head_skb ? chunk->head_skb : chunk->skb); > consume_skb(chunk->auth_chunk); > > SCTP_DBG_OBJCNT_DEC(chunk); > -- > 2.43.0 >
On Mon, 2024-02-05 at 18:29 -0500, Xin Long wrote: > In this case, sctp_inq_free() should take care of its release, > so can you try to fix it there? like: Yes, this seems fixes the leak(s) as well. Won't insist on my version assuming that you know better. Dmitry
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c index f80208edd6a5..30fe34743009 100644 --- a/net/sctp/sm_make_chunk.c +++ b/net/sctp/sm_make_chunk.c @@ -1500,7 +1500,10 @@ static void sctp_chunk_destroy(struct sctp_chunk *chunk) BUG_ON(!list_empty(&chunk->list)); list_del_init(&chunk->transmitted_list); - consume_skb(chunk->skb); + /* In case of GSO, 'skb' may be a pointer to fraglist entry. + * Consume the read head if so. + */ + consume_skb(chunk->head_skb ? chunk->head_skb : chunk->skb); consume_skb(chunk->auth_chunk); SCTP_DBG_OBJCNT_DEC(chunk);
In case of GSO, per-chunk 'skb' pointer may point to an entry from fraglist created in 'sctp_packet_gso_append()'. To avoid freeing random fraglist entry (and so undefined behavior and/or memory leak), consume 'head_skb' (i.e. beginning of a fraglist) instead. Reported-by: syzbot+8bb053b5d63595ab47db@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?id=0d8351bbe54fd04a492c2daab0164138db008042 Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru> --- net/sctp/sm_make_chunk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)