* [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 13:45 Eric Dumazet 2017-10-17 15:31 ` Xin Long 0 siblings, 1 reply; 17+ messages in thread From: Eric Dumazet @ 2017-10-17 13:45 UTC (permalink / raw) To: Vlad Yasevich, Neil Horman, Xin Long, Marcelo Ricardo Leitner Cc: netdev, Wei Wang, edumazet 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. 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(); } ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 13:45 [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() Eric Dumazet @ 2017-10-17 15:31 ` Xin Long 0 siblings, 0 replies; 17+ messages in thread From: Xin Long @ 2017-10-17 15:31 UTC (permalink / raw) To: Eric Dumazet Cc: Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, netdev, Wei Wang, Eric Dumazet, linux-sctp 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(); > } > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 15:31 ` Xin Long 0 siblings, 0 replies; 17+ messages in thread From: Xin Long @ 2017-10-17 15:31 UTC (permalink / raw) To: Eric Dumazet Cc: Vlad Yasevich, Neil Horman, Marcelo Ricardo Leitner, netdev, Wei Wang, Eric Dumazet, linux-sctp 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(); > } > > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 15:31 ` Xin Long @ 2017-10-17 16:28 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 16:28 UTC (permalink / raw) To: Xin Long Cc: Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, Eric Dumazet, linux-sctp 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 16:28 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 16:28 UTC (permalink / raw) To: Xin Long Cc: Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, Eric Dumazet, linux-sctp 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 16:28 ` Marcelo Ricardo Leitner @ 2017-10-17 16:44 ` Eric Dumazet -1 siblings, 0 replies; 17+ messages in thread From: Eric Dumazet @ 2017-10-17 16:44 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 16:44 ` Eric Dumazet 0 siblings, 0 replies; 17+ messages in thread From: Eric Dumazet @ 2017-10-17 16:44 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 16:44 ` Eric Dumazet @ 2017-10-17 17:01 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 17:01 UTC (permalink / raw) To: Eric Dumazet Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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; } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 17:01 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 17:01 UTC (permalink / raw) To: Eric Dumazet Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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; } ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 17:01 ` Marcelo Ricardo Leitner @ 2017-10-17 17:20 ` Eric Dumazet -1 siblings, 0 replies; 17+ messages in thread From: Eric Dumazet @ 2017-10-17 17:20 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 17:20 ` Eric Dumazet 0 siblings, 0 replies; 17+ messages in thread From: Eric Dumazet @ 2017-10-17 17:20 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 17:20 ` Eric Dumazet @ 2017-10-17 17:27 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 17:27 UTC (permalink / raw) To: Eric Dumazet Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 17:27 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-17 17:27 UTC (permalink / raw) To: Eric Dumazet Cc: Xin Long, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 17:27 ` Marcelo Ricardo Leitner @ 2017-10-17 17:33 ` Xin Long -1 siblings, 0 replies; 17+ messages in thread From: Xin Long @ 2017-10-17 17:33 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Eric Dumazet, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-17 17:33 ` Xin Long 0 siblings, 0 replies; 17+ messages in thread From: Xin Long @ 2017-10-17 17:33 UTC (permalink / raw) To: Marcelo Ricardo Leitner Cc: Eric Dumazet, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() 2017-10-17 17:33 ` Xin Long @ 2017-10-18 10:01 ` Marcelo Ricardo Leitner -1 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-18 10:01 UTC (permalink / raw) To: Xin Long Cc: Eric Dumazet, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() @ 2017-10-18 10:01 ` Marcelo Ricardo Leitner 0 siblings, 0 replies; 17+ messages in thread From: Marcelo Ricardo Leitner @ 2017-10-18 10:01 UTC (permalink / raw) To: Xin Long Cc: Eric Dumazet, Eric Dumazet, Vlad Yasevich, Neil Horman, netdev, Wei Wang, linux-sctp 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2017-10-18 10:01 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-17 13:45 [RFC] sctp: suspicious rcu_read_lock() in sctp_packet_config() Eric Dumazet 2017-10-17 15:31 ` Xin Long 2017-10-17 15:31 ` Xin Long 2017-10-17 16:28 ` Marcelo Ricardo Leitner 2017-10-17 16:28 ` Marcelo Ricardo Leitner 2017-10-17 16:44 ` Eric Dumazet 2017-10-17 16:44 ` Eric Dumazet 2017-10-17 17:01 ` Marcelo Ricardo Leitner 2017-10-17 17:01 ` Marcelo Ricardo Leitner 2017-10-17 17:20 ` Eric Dumazet 2017-10-17 17:20 ` Eric Dumazet 2017-10-17 17:27 ` Marcelo Ricardo Leitner 2017-10-17 17:27 ` Marcelo Ricardo Leitner 2017-10-17 17:33 ` Xin Long 2017-10-17 17:33 ` Xin Long 2017-10-18 10:01 ` Marcelo Ricardo Leitner 2017-10-18 10:01 ` Marcelo Ricardo Leitner
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.