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

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)


BTW, sparse throws a lot of errors, any volunteer to clean this mess ?

make C=2 M=net/sctp

Thanks.

On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> 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.

>
>
> BTW, sparse throws a lot of errors, any volunteer to clean this mess ?
will do it.

Thanks for reporting this.

>
> make C=2 M=net/sctp
>
> Thanks.
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index 4a865cd06d76cd5b2aa417de618da3203f7b53e4..d7f320f5acc271189ec9474795b6ececed7ad2b9 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -86,6 +86,7 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag,
>  {
>         struct sctp_transport *tp = packet->transport;
>         struct sctp_association *asoc = tp->asoc;
> +       struct dst_entry *dst;
>         struct sock *sk;
>
>         pr_debug("%s: packet:%p vtag:0x%x\n", __func__, packet, vtag);
> @@ -121,17 +122,15 @@ void sctp_packet_config(struct sctp_packet *packet, __u32 vtag,
>                         sctp_packet_append_chunk(packet, chunk);
>         }
>
> -       if (!tp->dst)
> -               return;
> -
>         /* set packet max_size with gso_max_size if gso is enabled*/
>         rcu_read_lock();
> -       if (__sk_dst_get(sk) != tp->dst) {
> -               dst_hold(tp->dst);
> -               sk_setup_caps(sk, tp->dst);
> +       dst = rcu_dereference(tp->dst);
> +       if (dst) {
> +               if (__sk_dst_get(sk) != dst && dst_hold_safe(dst))
> +                       sk_setup_caps(sk, dst);
> +               packet->max_size = sk_can_gso(sk) ? dst->dev->gso_max_size
> +                                                 : asoc->pathmtu;
>         }
> -       packet->max_size = sk_can_gso(sk) ? tp->dst->dev->gso_max_size
> -                                         : asoc->pathmtu;
>         rcu_read_unlock();
>  }
>
>
>

On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> > 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().  It's not possible that these would trip
on the update going on on sctp_packet_config() because the socket is
locked. We may not need (much) more than the example patch, I think.

A more thorough check is certainly welcomed, indeed.

  Marcelo

On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
>> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> > 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 ?

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.

On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:
> On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
> >> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> > 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,

> 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;
	}

On Tue, Oct 17, 2017 at 10:01 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:
>> On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>> > On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
>> >> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >> > 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,
>
>> 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.



I

On Tue, Oct 17, 2017 at 10:20:58AM -0700, Eric Dumazet wrote:
> On Tue, Oct 17, 2017 at 10:01 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:
> >> On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
> >> <marcelo.leitner@gmail.com> wrote:
> >> > On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
> >> >> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> >> > 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,
> >
> >> 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.

Will look more, thanks.

On Wed, Oct 18, 2017 at 1:27 AM, Marcelo Ricardo Leitner
<marcelo.leitner@gmail.com> wrote:
> On Tue, Oct 17, 2017 at 10:20:58AM -0700, Eric Dumazet wrote:
>> On Tue, Oct 17, 2017 at 10:01 AM, Marcelo Ricardo Leitner
>> <marcelo.leitner@gmail.com> wrote:
>> > On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:
>> >> On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
>> >> <marcelo.leitner@gmail.com> wrote:
>> >> > On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
>> >> >> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> >> >> > 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,
>> >
>> >> 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;

>
> Will look more, thanks.
>

On Wed, Oct 18, 2017 at 01:33:46AM +0800, Xin Long wrote:
> On Wed, Oct 18, 2017 at 1:27 AM, Marcelo Ricardo Leitner
> <marcelo.leitner@gmail.com> wrote:
> > On Tue, Oct 17, 2017 at 10:20:58AM -0700, Eric Dumazet wrote:
> >> On Tue, Oct 17, 2017 at 10:01 AM, Marcelo Ricardo Leitner
> >> <marcelo.leitner@gmail.com> wrote:
> >> > On Tue, Oct 17, 2017 at 09:44:10AM -0700, Eric Dumazet wrote:
> >> >> On Tue, Oct 17, 2017 at 9:28 AM, Marcelo Ricardo Leitner
> >> >> <marcelo.leitner@gmail.com> wrote:
> >> >> > On Tue, Oct 17, 2017 at 11:31:30PM +0800, Xin Long wrote:
> >> >> >> On Tue, Oct 17, 2017 at 9:45 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> >> >> >> > 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,
> >> >
> >> >> 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;

Looks like it.