netdev - Re: [PATCH v5] sctp: fix refcount bug in sctp

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] [day] [month] [year] [list]

Date:   Fri, 27 Mar 2020 09:26:24 +0800
From:   Qiujun Huang <hqjagain@...il.com>
To:     Marcelo Ricardo Leitner <marcelo.leitner@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     vyasevich@...il.com, nhorman@...driver.com,
        Jakub Kicinski <kuba@...nel.org>, linux-sctp@...r.kernel.org,
        netdev <netdev@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, anenbupt@...il.com
Subject: Re: [PATCH v5] sctp: fix refcount bug in sctp_wfree

sorry about missing a line. please ignore this.
I'll resend the patch.

On Fri, Mar 27, 2020 at 9:19 AM Qiujun Huang <hqjagain@...il.com> wrote:
>
> We should iterate over the datamsgs to modify
> all chunks(skbs) to newsk.
>
> The following case cause the bug:
> for the trouble SKB, it was in outq->transmitted list
>
> sctp_outq_sack
>         sctp_check_transmitted
>                 SKB was moved to outq->sacked list
>         then throw away the sack queue
>                 SKB was deleted from outq->sacked
> (but it was held by datamsg at sctp_datamsg_to_asoc
> So, sctp_wfree was not called here)
>
> then migrate happened
>
>         sctp_for_each_tx_datachunk(
>         sctp_clear_owner_w);
>         sctp_assoc_migrate();
>         sctp_for_each_tx_datachunk(
>         sctp_set_owner_w);
> SKB was not in the outq, and was not changed to newsk
>
> finally
>
> __sctp_outq_teardown
>         sctp_chunk_put (for another skb)
>                 sctp_datamsg_put
>                         __kfree_skb(msg->frag_list)
>                                 sctp_wfree (for SKB)
>         SKB->sk was still oldsk (skb->sk != asoc->base.sk).
>
> Reported-and-tested-by:syzbot+cea71eec5d6de256d54d@...kaller.appspotmail.com
> Signed-off-by: Qiujun Huang <hqjagain@...il.com>
> ---
>  net/sctp/socket.c | 30 ++++++++++++++++++++++--------
>  1 file changed, 22 insertions(+), 8 deletions(-)
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 1b56fc440606..75acbd5d4597 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -147,29 +147,43 @@ static void sctp_clear_owner_w(struct sctp_chunk *chunk)
>         skb_orphan(chunk->skb);
>  }
>
> +#define traverse_and_process() \
> +do {                           \
> +       msg = chunk->msg;       \
> +       if (msg == prev_msg)    \
> +               continue;       \
> +       list_for_each_entry(c, &msg->chunks, frag_list) {       \
> +               if ((clear && asoc->base.sk == c->skb->sk) ||   \
> +                   (!clear && asoc->base.sk != c->skb->sk))    \
> +                   cb(c);      \
> +       }                       \
> +} while (0)
> +
>  static void sctp_for_each_tx_datachunk(struct sctp_association *asoc,
> +                                      bool clear,
>                                        void (*cb)(struct sctp_chunk *))
>
>  {
> +       struct sctp_datamsg *msg, *prev_msg = NULL;
>         struct sctp_outq *q = &asoc->outqueue;
> +       struct sctp_chunk *chunk, *c;
>         struct sctp_transport *t;
> -       struct sctp_chunk *chunk;
>
>         list_for_each_entry(t, &asoc->peer.transport_addr_list, transports)
>                 list_for_each_entry(chunk, &t->transmitted, transmitted_list)
> -                       cb(chunk);
> +                       traverse_and_process();
>
>         list_for_each_entry(chunk, &q->retransmit, transmitted_list)
> -               cb(chunk);
> +               traverse_and_process();
>
>         list_for_each_entry(chunk, &q->sacked, transmitted_list)
> -               cb(chunk);
> +               traverse_and_process();
>
>         list_for_each_entry(chunk, &q->abandoned, transmitted_list)
> -               cb(chunk);
> +               traverse_and_process();
>
>         list_for_each_entry(chunk, &q->out_chunk_list, list)
> -               cb(chunk);
> +               traverse_and_process();
>  }
>
>  static void sctp_for_each_rx_skb(struct sctp_association *asoc, struct sock *sk,
> @@ -9574,9 +9588,9 @@ static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>          * paths won't try to lock it and then oldsk.
>          */
>         lock_sock_nested(newsk, SINGLE_DEPTH_NESTING);
> -       sctp_for_each_tx_datachunk(assoc, sctp_clear_owner_w);
> +       sctp_for_each_tx_datachunk(assoc, true, sctp_clear_owner_w);
>         sctp_assoc_migrate(assoc, newsk);
> -       sctp_for_each_tx_datachunk(assoc, sctp_set_owner_w);
> +       sctp_for_each_tx_datachunk(assoc, false, sctp_set_owner_w);
>
>         /* If the association on the newsk is already closed before accept()
>          * is called, set RCV_SHUTDOWN flag.
> --
> 2.17.1
>