| 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
| ||
|
Message-ID: <20161031201508.GE8514@localhost.localdomain>
Date: Mon, 31 Oct 2016 18:15:08 -0200
From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
To: Xin Long <lucien.xin@...il.com>
Cc: network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
davem@...emloft.net, Vlad Yasevich <vyasevich@...il.com>,
Neil Horman <nhorman@...driver.com>
Subject: Re: [PATCHv2 net 3/3] sctp: hold transport instead of assoc when
lookup assoc in rx path
On Mon, Oct 31, 2016 at 08:32:33PM +0800, Xin Long wrote:
> Prior to this patch, in rx path, before calling lock_sock, it needed to
> hold assoc when got it by __sctp_lookup_association, in case other place
> would free/put assoc.
>
> But in __sctp_lookup_association, it lookup and hold transport, then got
> assoc by transport->assoc, then hold assoc and put transport. It means
> it didn't hold transport, yet it was returned and later on directly
> assigned to chunk->transport.
>
> Without the protection of sock lock, the transport may be freed/put by
> other places, which would cause a use-after-free issue.
>
> This patch is to fix this issue by holding transport instead of assoc.
> As holding transport can make sure to access assoc is also safe, and
> actually it looks up assoc by searching transport rhashtable, to hold
> transport here makes more sense.
>
> Note that the function will be renamed later on on another patch.
>
> Signed-off-by: Xin Long <lucien.xin@...il.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Thanks
> ---
> include/net/sctp/sctp.h | 2 +-
> net/sctp/input.c | 32 ++++++++++++++++----------------
> net/sctp/ipv6.c | 2 +-
> 3 files changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
> index 87a7f42..31acc3f 100644
> --- a/include/net/sctp/sctp.h
> +++ b/include/net/sctp/sctp.h
> @@ -152,7 +152,7 @@ void sctp_unhash_endpoint(struct sctp_endpoint *);
> struct sock *sctp_err_lookup(struct net *net, int family, struct sk_buff *,
> struct sctphdr *, struct sctp_association **,
> struct sctp_transport **);
> -void sctp_err_finish(struct sock *, struct sctp_association *);
> +void sctp_err_finish(struct sock *, struct sctp_transport *);
> void sctp_icmp_frag_needed(struct sock *, struct sctp_association *,
> struct sctp_transport *t, __u32 pmtu);
> void sctp_icmp_redirect(struct sock *, struct sctp_transport *,
> diff --git a/net/sctp/input.c b/net/sctp/input.c
> index 8e0bc58..a01a56e 100644
> --- a/net/sctp/input.c
> +++ b/net/sctp/input.c
> @@ -181,9 +181,10 @@ int sctp_rcv(struct sk_buff *skb)
> * bound to another interface, via SO_BINDTODEVICE, treat it as OOTB
> */
> if (sk->sk_bound_dev_if && (sk->sk_bound_dev_if != af->skb_iif(skb))) {
> - if (asoc) {
> - sctp_association_put(asoc);
> + if (transport) {
> + sctp_transport_put(transport);
> asoc = NULL;
> + transport = NULL;
> } else {
> sctp_endpoint_put(ep);
> ep = NULL;
> @@ -269,8 +270,8 @@ int sctp_rcv(struct sk_buff *skb)
> bh_unlock_sock(sk);
>
> /* Release the asoc/ep ref we took in the lookup calls. */
> - if (asoc)
> - sctp_association_put(asoc);
> + if (transport)
> + sctp_transport_put(transport);
> else
> sctp_endpoint_put(ep);
>
> @@ -283,8 +284,8 @@ int sctp_rcv(struct sk_buff *skb)
>
> discard_release:
> /* Release the asoc/ep ref we took in the lookup calls. */
> - if (asoc)
> - sctp_association_put(asoc);
> + if (transport)
> + sctp_transport_put(transport);
> else
> sctp_endpoint_put(ep);
>
> @@ -300,6 +301,7 @@ int sctp_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> {
> struct sctp_chunk *chunk = SCTP_INPUT_CB(skb)->chunk;
> struct sctp_inq *inqueue = &chunk->rcvr->inqueue;
> + struct sctp_transport *t = chunk->transport;
> struct sctp_ep_common *rcvr = NULL;
> int backloged = 0;
>
> @@ -351,7 +353,7 @@ int sctp_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> done:
> /* Release the refs we took in sctp_add_backlog */
> if (SCTP_EP_TYPE_ASSOCIATION == rcvr->type)
> - sctp_association_put(sctp_assoc(rcvr));
> + sctp_transport_put(t);
> else if (SCTP_EP_TYPE_SOCKET == rcvr->type)
> sctp_endpoint_put(sctp_ep(rcvr));
> else
> @@ -363,6 +365,7 @@ int sctp_backlog_rcv(struct sock *sk, struct sk_buff *skb)
> static int sctp_add_backlog(struct sock *sk, struct sk_buff *skb)
> {
> struct sctp_chunk *chunk = SCTP_INPUT_CB(skb)->chunk;
> + struct sctp_transport *t = chunk->transport;
> struct sctp_ep_common *rcvr = chunk->rcvr;
> int ret;
>
> @@ -373,7 +376,7 @@ static int sctp_add_backlog(struct sock *sk, struct sk_buff *skb)
> * from us
> */
> if (SCTP_EP_TYPE_ASSOCIATION == rcvr->type)
> - sctp_association_hold(sctp_assoc(rcvr));
> + sctp_transport_hold(t);
> else if (SCTP_EP_TYPE_SOCKET == rcvr->type)
> sctp_endpoint_hold(sctp_ep(rcvr));
> else
> @@ -537,15 +540,15 @@ struct sock *sctp_err_lookup(struct net *net, int family, struct sk_buff *skb,
> return sk;
>
> out:
> - sctp_association_put(asoc);
> + sctp_transport_put(transport);
> return NULL;
> }
>
> /* Common cleanup code for icmp/icmpv6 error handler. */
> -void sctp_err_finish(struct sock *sk, struct sctp_association *asoc)
> +void sctp_err_finish(struct sock *sk, struct sctp_transport *t)
> {
> bh_unlock_sock(sk);
> - sctp_association_put(asoc);
> + sctp_transport_put(t);
> }
>
> /*
> @@ -641,7 +644,7 @@ void sctp_v4_err(struct sk_buff *skb, __u32 info)
> }
>
> out_unlock:
> - sctp_err_finish(sk, asoc);
> + sctp_err_finish(sk, transport);
> }
>
> /*
> @@ -952,11 +955,8 @@ static struct sctp_association *__sctp_lookup_association(
> goto out;
>
> asoc = t->asoc;
> - sctp_association_hold(asoc);
> *pt = t;
>
> - sctp_transport_put(t);
> -
> out:
> return asoc;
> }
> @@ -986,7 +986,7 @@ int sctp_has_association(struct net *net,
> struct sctp_transport *transport;
>
> if ((asoc = sctp_lookup_association(net, laddr, paddr, &transport))) {
> - sctp_association_put(asoc);
> + sctp_transport_put(transport);
> return 1;
> }
>
> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
> index f473779..176af30 100644
> --- a/net/sctp/ipv6.c
> +++ b/net/sctp/ipv6.c
> @@ -198,7 +198,7 @@ static void sctp_v6_err(struct sk_buff *skb, struct inet6_skb_parm *opt,
> }
>
> out_unlock:
> - sctp_err_finish(sk, asoc);
> + sctp_err_finish(sk, transport);
> out:
> if (likely(idev != NULL))
> in6_dev_put(idev);
> --
> 2.1.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-sctp" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Powered by blists - more mailing lists