netdev - Re: [PATCH] net: sctp: fix memory leak in sctp_chunk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CADvbK_fn+gH=p-OhVXzZtGd+nK6QUKu+F4QLBpcx0c3Pig1oLg@mail.gmail.com>
Date: Mon, 5 Feb 2024 18:29:24 -0500
From: Xin Long <lucien.xin@...il.com>
To: Dmitry Antipov <dmantipov@...dex.ru>
Cc: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>, linux-sctp@...r.kernel.org, 
	netdev@...r.kernel.org, lvc-project@...uxtesting.org, 
	syzbot+8bb053b5d63595ab47db@...kaller.appspotmail.com
Subject: Re: [PATCH] net: sctp: fix memory leak in sctp_chunk_destroy()

On Mon, Feb 5, 2024 at 12:02 PM Dmitry Antipov <dmantipov@...dex.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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?id=0d8351bbe54fd04a492c2daab0164138db008042
> Signed-off-by: Dmitry Antipov <dmantipov@...dex.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
>