netdev - Re: [PATCH v2] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200318035549.GC3756@localhost.localdomain>
Date:   Wed, 18 Mar 2020 00:55:49 -0300
From:   Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To:     Qiujun Huang <hqjagain@...il.com>
Cc:     "David S. Miller" <davem@...emloft.net>, 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 v2] sctp: fix refcount bug in sctp_wfree

On Wed, Mar 18, 2020 at 10:45:51AM +0800, Qiujun Huang wrote:
> On Wed, Mar 18, 2020 at 1:30 AM Marcelo Ricardo Leitner
> <marcelo.leitner@...il.com> wrote:
> >
> > Hi,
> >
> > On Tue, Mar 17, 2020 at 11:55:36PM +0800, Qiujun Huang wrote:
> > > Do accounting for skb's real sk.
> > > In some case skb->sk != asoc->base.sk:
> > >
> > > migrate routing        sctp_check_transmitted routing
> > > ------------                    ---------------
> >                                  sctp_close();
> >                                    lock_sock(sk2);
> >                                  sctp_primitive_ABORT();
> >                                  sctp_do_sm();
> >                                  sctp_cmd_interpreter();
> >                                  sctp_cmd_process_sack();
> >                                  sctp_outq_sack();
> >                                  sctp_check_transmitted();
> >
> >   lock_sock(sk1);
> >   sctp_getsockopt_peeloff();
> >   sctp_do_peeloff();
> >   sctp_sock_migrate();
> > > lock_sock_nested(sk2);
> > >                                mv the transmitted skb to
> > >                                the it's local tlist
> >
> >
> > How can sctp_do_sm() be called in the 2nd column so that it bypasses
> > the locks in the left column, allowing this mv to happen?
> >
> > >
> > > sctp_for_each_tx_datachunk(
> > > sctp_clear_owner_w);
> > > sctp_assoc_migrate();
> > > sctp_for_each_tx_datachunk(
> > > sctp_set_owner_w);
> > >
> > >                                put the skb back to the
> > >                                assoc lists
> > > ----------------------------------------------------
> > >
> > > The skbs which held bysctp_check_transmitted were not changed
> > > to newsk. They were not dealt with by sctp_for_each_tx_datachunk
> > > (sctp_clear_owner_w/sctp_set_owner_w).
> >
> > It would make sense but I'm missing one step earlier, I'm not seeing
> > how the move to local list is allowed/possible in there. It really
> > shouldn't be possible.
> 
> I totally agree that.
> My mistake. So I added some log in my test showing the case:
> The backtrace:
> sctp_close
> sctp_primitive_ABORT
> sctp_do_sm
> sctp_association_free
> __sctp_outq_teardown
>      /* Throw away unacknowledged chunks. */
>     list_for_each_entry(transport, &q->asoc->peer.transport_addr_list,
>     transports) {
>     printk("[%d]deal with transmitted %#llx from transport %#llx  %s,
> %d\n", raw_smp_processor_id(),
>    &transport->transmitted, transport, __func__, __LINE__);
>    while ((lchunk = sctp_list_dequeue(&transport->transmitted)) != NULL) {
> 
> The trouble skb is from another peer sk in the same asoc, but
> accounted to the base.sk.

Hmm, not sure how you got that out of that debug msg, but okay.
Even if so, how would this trouble skb be accounted on the wrong sk by
then?

Asking because the fix that we want may be a better locking, to
prevent this situation from happening, than compensating for it in
sctp_wfree(). But for that we need to understand how this happened.

  Marcelo