Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config()

On Wed, Oct 18, 2017 at 1:27 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

On Tue, Oct 17, 2017 at 10:20:58AM -0700, Eric Dumazet wrote:

quoted

On Tue, Oct 17, 2017 at 10:01 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:

quoted

On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
[off-list ref] wrote:

quoted

On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:

quoted

On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet [off-list ref] wrote:

quoted

SCTP experts.

syszkaller reported a few crashes in sctp_packet_config() with invalid
access to a deleted dst.

The rcu_read_lock() in sctp_packet_config() is suspect.

It does not protect anything at the moment.

If we expect tp->dst to be manipulated/changed by another cpu/thread,
then we need proper rcu protection.

Following patch to show what would be a minimal change (but obviously
bigger changes are needed, like sctp_transport_pmtu_check() and
sctp_transport_dst_check(), and proper sparse annotations)

will check all places accessing tp->dst in sctp.

I checked some and sctp_transport_dst_check() should be fine because
by then we are holding a reference on dst. Same goes to
sctp_transport_pmtu_check().

Really ?

Yes,

quoted

What about sctp_v4_err() -> sctp_icmp_redirect() -> sctp_transport_dst_check()

It seems quite possible that the BH handler can access it, while
socket is owned by user.

hidden here:
sctp_v4_err() {
...
        sk = sctp_err_lookup(net, AF_INET, skb, sctp_hdr(skb), &asoc,
        &transport);
...
out_unlock:
        sctp_err_finish(sk, transport);
}

sctp_err_lookup() {
...
        bh_lock_sock(sk);

        /* If too many ICMPs get dropped on busy
         * servers this needs to be solved differently.
         */
        if (sock_owned_by_user(sk))            [A]
                __NET_INC_STATS(net, LINUX_MIB_LOCKDROPPEDICMPS);

        *app = asoc;
        *tpp = transport;
        return sk;
...
}

Though that if() on [A] should be bailing out without returning
nothing. That's a bug. More like:

        if (sock_owned_by_user(sk)) {
                __NET_INC_STATS(net, LINUX_MIB_LOCKDROPPEDICMPS);
                goto out;
        }

So why sctp_v4_err() is doing this test ?

if (!sock_owned_by_user(sk) && inet->recverr) {

It looks like socket can be owned by the user, and [A] check only
increments an SNMP counter,
that wont help to solve the tp->dst use after free.

Hah, missed that. Though the semantics on that counter still looks
confusing. It may be incremented when we actually handled the icmp.
The other icmp handling in there will postpone in case the socket is
locked by the user, and so will the timer callbacks too.

Maybe that check should be done in sctp_icmp_redirect(), as
in sctp_icmp_frag_needed(), as well as in tcp_v4_err().

@@ -421,7 +421,7 @@ void sctp_icmp_redirect(struct sock *sk, struct

sctp_transport *t,
 {
        struct dst_entry *dst;

-       if (!t)
+       if (sock_owned_by_user(sk) || !t)
                return;