| From foo@baz Fri 03 Apr 2020 10:57:34 AM CEST |
| From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| Date: Thu, 26 Mar 2020 20:47:46 -0300 |
| Subject: sctp: fix possibly using a bad saddr with a given dst |
| |
| From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| |
| [ Upstream commit 582eea230536a6f104097dd46205822005d5fe3a ] |
| |
| Under certain circumstances, depending on the order of addresses on the |
| interfaces, it could be that sctp_v[46]_get_dst() would return a dst |
| with a mismatched struct flowi. |
| |
| For example, if when walking through the bind addresses and the first |
| one is not a match, it saves the dst as a fallback (added in |
| 410f03831c07), but not the flowi. Then if the next one is also not a |
| match, the previous dst will be returned but with the flowi information |
| for the 2nd address, which is wrong. |
| |
| The fix is to use a locally stored flowi that can be used for such |
| attempts, and copy it to the parameter only in case it is a possible |
| match, together with the corresponding dst entry. |
| |
| The patch updates IPv6 code mostly just to be in sync. Even though the issue |
| is also present there, it fallback is not expected to work with IPv6. |
| |
| Fixes: 410f03831c07 ("sctp: add routing output fallback") |
| Reported-by: Jin Meng <meng.a.jin@nokia-sbell.com> |
| Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com> |
| Tested-by: Xin Long <lucien.xin@gmail.com> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| net/sctp/ipv6.c | 20 ++++++++++++++------ |
| net/sctp/protocol.c | 28 +++++++++++++++++++--------- |
| 2 files changed, 33 insertions(+), 15 deletions(-) |
| |
| --- a/net/sctp/ipv6.c |
| +++ b/net/sctp/ipv6.c |
| @@ -235,7 +235,8 @@ static void sctp_v6_get_dst(struct sctp_ |
| { |
| struct sctp_association *asoc = t->asoc; |
| struct dst_entry *dst = NULL; |
| - struct flowi6 *fl6 = &fl->u.ip6; |
| + struct flowi _fl; |
| + struct flowi6 *fl6 = &_fl.u.ip6; |
| struct sctp_bind_addr *bp; |
| struct ipv6_pinfo *np = inet6_sk(sk); |
| struct sctp_sockaddr_entry *laddr; |
| @@ -245,7 +246,7 @@ static void sctp_v6_get_dst(struct sctp_ |
| __u8 matchlen = 0; |
| sctp_scope_t scope; |
| |
| - memset(fl6, 0, sizeof(struct flowi6)); |
| + memset(&_fl, 0, sizeof(_fl)); |
| fl6->daddr = daddr->v6.sin6_addr; |
| fl6->fl6_dport = daddr->v6.sin6_port; |
| fl6->flowi6_proto = IPPROTO_SCTP; |
| @@ -269,8 +270,11 @@ static void sctp_v6_get_dst(struct sctp_ |
| rcu_read_unlock(); |
| |
| dst = ip6_dst_lookup_flow(sk, fl6, final_p); |
| - if (!asoc || saddr) |
| + if (!asoc || saddr) { |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| goto out; |
| + } |
| |
| bp = &asoc->base.bind_addr; |
| scope = sctp_scope(daddr); |
| @@ -293,6 +297,8 @@ static void sctp_v6_get_dst(struct sctp_ |
| if ((laddr->a.sa.sa_family == AF_INET6) && |
| (sctp_v6_cmp_addr(&dst_saddr, &laddr->a))) { |
| rcu_read_unlock(); |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| goto out; |
| } |
| } |
| @@ -331,6 +337,8 @@ static void sctp_v6_get_dst(struct sctp_ |
| if (!IS_ERR_OR_NULL(dst)) |
| dst_release(dst); |
| dst = bdst; |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| break; |
| } |
| |
| @@ -344,6 +352,8 @@ static void sctp_v6_get_dst(struct sctp_ |
| dst_release(dst); |
| dst = bdst; |
| matchlen = bmatchlen; |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| } |
| rcu_read_unlock(); |
| |
| @@ -352,14 +362,12 @@ out: |
| struct rt6_info *rt; |
| |
| rt = (struct rt6_info *)dst; |
| - t->dst = dst; |
| t->dst_cookie = rt6_get_cookie(rt); |
| pr_debug("rt6_dst:%pI6/%d rt6_src:%pI6\n", |
| &rt->rt6i_dst.addr, rt->rt6i_dst.plen, |
| - &fl6->saddr); |
| + &fl->u.ip6.saddr); |
| } else { |
| t->dst = NULL; |
| - |
| pr_debug("no route\n"); |
| } |
| } |
| --- a/net/sctp/protocol.c |
| +++ b/net/sctp/protocol.c |
| @@ -430,14 +430,15 @@ static void sctp_v4_get_dst(struct sctp_ |
| { |
| struct sctp_association *asoc = t->asoc; |
| struct rtable *rt; |
| - struct flowi4 *fl4 = &fl->u.ip4; |
| + struct flowi _fl; |
| + struct flowi4 *fl4 = &_fl.u.ip4; |
| struct sctp_bind_addr *bp; |
| struct sctp_sockaddr_entry *laddr; |
| struct dst_entry *dst = NULL; |
| union sctp_addr *daddr = &t->ipaddr; |
| union sctp_addr dst_saddr; |
| |
| - memset(fl4, 0x0, sizeof(struct flowi4)); |
| + memset(&_fl, 0x0, sizeof(_fl)); |
| fl4->daddr = daddr->v4.sin_addr.s_addr; |
| fl4->fl4_dport = daddr->v4.sin_port; |
| fl4->flowi4_proto = IPPROTO_SCTP; |
| @@ -455,8 +456,11 @@ static void sctp_v4_get_dst(struct sctp_ |
| &fl4->saddr); |
| |
| rt = ip_route_output_key(sock_net(sk), fl4); |
| - if (!IS_ERR(rt)) |
| + if (!IS_ERR(rt)) { |
| dst = &rt->dst; |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| + } |
| |
| /* If there is no association or if a source address is passed, no |
| * more validation is required. |
| @@ -519,27 +523,33 @@ static void sctp_v4_get_dst(struct sctp_ |
| odev = __ip_dev_find(sock_net(sk), laddr->a.v4.sin_addr.s_addr, |
| false); |
| if (!odev || odev->ifindex != fl4->flowi4_oif) { |
| - if (!dst) |
| + if (!dst) { |
| dst = &rt->dst; |
| - else |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| + } else { |
| dst_release(&rt->dst); |
| + } |
| continue; |
| } |
| |
| dst_release(dst); |
| dst = &rt->dst; |
| + t->dst = dst; |
| + memcpy(fl, &_fl, sizeof(_fl)); |
| break; |
| } |
| |
| out_unlock: |
| rcu_read_unlock(); |
| out: |
| - t->dst = dst; |
| - if (dst) |
| + if (dst) { |
| pr_debug("rt_dst:%pI4, rt_src:%pI4\n", |
| - &fl4->daddr, &fl4->saddr); |
| - else |
| + &fl->u.ip4.daddr, &fl->u.ip4.saddr); |
| + } else { |
| + t->dst = NULL; |
| pr_debug("no route\n"); |
| + } |
| } |
| |
| /* For v4, the source address is cached in the route entry(dst). So no need |