[{"id":1769163,"web_url":"http://patchwork.ozlabs.org/comment/1769163/","msgid":"<20170915125500.GC4529@localhost.localdomain>","list_archive_url":null,"date":"2017-09-15T12:55:00","subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","submitter":{"id":65333,"url":"http://patchwork.ozlabs.org/api/people/65333/","name":"Marcelo Ricardo Leitner","email":"marcelo.leitner@gmail.com"},"content":"On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:\n> Commit 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the\n> dump\") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep\n> with holding sock and sock lock.\n> \n> But Paolo noticed that endpoint could be destroyed in sctp_rcv without\n> sock lock protection. It means the use-after-free issue still could be\n> triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks\n> !ep, although it's pretty hard to reproduce.\n> \n> I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close\n> and sctp_sock_dump long time.\n> \n> This patch is to add another param cb_done to sctp_for_each_transport\n> and dump ep->assocs with holding tsp after jumping out of transport's\n> traversal in it to avoid this issue.\n> \n> It can also improve sctp diag dump to make it run faster, as no need\n> to save sk into cb->args[5] and keep calling sctp_for_each_transport\n> any more.\n> \n> This patch is also to use int * instead of int for the pos argument\n> in sctp_for_each_transport, which could make postion increment only\n> in sctp_for_each_transport and no need to keep changing cb->args[2]\n> in sctp_sock_filter and sctp_sock_dump any more.\n> \n> Fixes: 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the dump\")\n> Reported-by: Paolo Abeni \n> Signed-off-by: Xin Long \n\nAcked-by: Marcelo Ricardo Leitner \n\n> ---\n> include/net/sctp/sctp.h | 3 ++-\n> net/sctp/sctp_diag.c | 32 +++++++++-----------------------\n> net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------\n> 3 files changed, 36 insertions(+), 39 deletions(-)\n> \n> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h\n> index 06b4f51..d7d8cba 100644\n> --- a/include/net/sctp/sctp.h\n> +++ b/include/net/sctp/sctp.h\n> @@ -127,7 +127,8 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),\n> \t\t\t\t const union sctp_addr *laddr,\n> \t\t\t\t const union sctp_addr *paddr, void *p);\n> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),\n> -\t\t\t struct net *net, int pos, void *p);\n> +\t\t\t int (*cb_done)(struct sctp_transport *, void *),\n> +\t\t\t struct net *net, int *pos, void *p);\n> int sctp_for_each_endpoint(int (*cb)(struct sctp_endpoint *, void *), void *p);\n> int sctp_get_sctp_info(struct sock *sk, struct sctp_association *asoc,\n> \t\t struct sctp_info *info);\n> diff --git a/net/sctp/sctp_diag.c b/net/sctp/sctp_diag.c\n> index e99518e..7008a99 100644\n> --- a/net/sctp/sctp_diag.c\n> +++ b/net/sctp/sctp_diag.c\n> @@ -279,9 +279,11 @@ static int sctp_tsp_dump_one(struct sctp_transport *tsp, void *p)\n> \treturn err;\n> }\n> \n> -static int sctp_sock_dump(struct sock *sk, void *p)\n> +static int sctp_sock_dump(struct sctp_transport *tsp, void *p)\n> {\n> +\tstruct sctp_endpoint *ep = tsp->asoc->ep;\n> \tstruct sctp_comm_param *commp = p;\n> +\tstruct sock *sk = ep->base.sk;\n> \tstruct sk_buff *skb = commp->skb;\n> \tstruct netlink_callback *cb = commp->cb;\n> \tconst struct inet_diag_req_v2 *r = commp->r;\n> @@ -289,9 +291,7 @@ static int sctp_sock_dump(struct sock *sk, void *p)\n> \tint err = 0;\n> \n> \tlock_sock(sk);\n> -\tif (!sctp_sk(sk)->ep)\n> -\t\tgoto release;\n> -\tlist_for_each_entry(assoc, &sctp_sk(sk)->ep->asocs, asocs) {\n> +\tlist_for_each_entry(assoc, &ep->asocs, asocs) {\n> \t\tif (cb->args[4] < cb->args[1])\n> \t\t\tgoto next;\n> \n> @@ -327,40 +327,30 @@ static int sctp_sock_dump(struct sock *sk, void *p)\n> \t\tcb->args[4]++;\n> \t}\n> \tcb->args[1] = 0;\n> -\tcb->args[2]++;\n> \tcb->args[3] = 0;\n> \tcb->args[4] = 0;\n> release:\n> \trelease_sock(sk);\n> -\tsock_put(sk);\n> \treturn err;\n> }\n> \n> -static int sctp_get_sock(struct sctp_transport *tsp, void *p)\n> +static int sctp_sock_filter(struct sctp_transport *tsp, void *p)\n> {\n> \tstruct sctp_endpoint *ep = tsp->asoc->ep;\n> \tstruct sctp_comm_param *commp = p;\n> \tstruct sock *sk = ep->base.sk;\n> -\tstruct netlink_callback *cb = commp->cb;\n> \tconst struct inet_diag_req_v2 *r = commp->r;\n> \tstruct sctp_association *assoc =\n> \t\tlist_entry(ep->asocs.next, struct sctp_association, asocs);\n> \n> \t/* find the ep only once through the transports by this condition */\n> \tif (tsp->asoc != assoc)\n> -\t\tgoto out;\n> +\t\treturn 0;\n> \n> \tif (r->sdiag_family != AF_UNSPEC && sk->sk_family != r->sdiag_family)\n> -\t\tgoto out;\n> -\n> -\tsock_hold(sk);\n> -\tcb->args[5] = (long)sk;\n> +\t\treturn 0;\n> \n> \treturn 1;\n> -\n> -out:\n> -\tcb->args[2]++;\n> -\treturn 0;\n> }\n> \n> static int sctp_ep_dump(struct sctp_endpoint *ep, void *p)\n> @@ -503,12 +493,8 @@ static void sctp_diag_dump(struct sk_buff *skb, struct netlink_callback *cb,\n> \tif (!(idiag_states & ~(TCPF_LISTEN | TCPF_CLOSE)))\n> \t\tgoto done;\n> \n> -next:\n> -\tcb->args[5] = 0;\n> -\tsctp_for_each_transport(sctp_get_sock, net, cb->args[2], &commp);\n> -\n> -\tif (cb->args[5] && !sctp_sock_dump((struct sock *)cb->args[5], &commp))\n> -\t\tgoto next;\n> +\tsctp_for_each_transport(sctp_sock_filter, sctp_sock_dump,\n> +\t\t\t\tnet, (int *)&cb->args[2], &commp);\n> \n> done:\n> \tcb->args[1] = cb->args[4];\n> diff --git a/net/sctp/socket.c b/net/sctp/socket.c\n> index 1b00a1e..d4730ad 100644\n> --- a/net/sctp/socket.c\n> +++ b/net/sctp/socket.c\n> @@ -4658,29 +4658,39 @@ int sctp_transport_lookup_process(int (*cb)(struct sctp_transport *, void *),\n> EXPORT_SYMBOL_GPL(sctp_transport_lookup_process);\n> \n> int sctp_for_each_transport(int (*cb)(struct sctp_transport *, void *),\n> -\t\t\t struct net *net, int pos, void *p) {\n> +\t\t\t int (*cb_done)(struct sctp_transport *, void *),\n> +\t\t\t struct net *net, int *pos, void *p) {\n> \tstruct rhashtable_iter hti;\n> -\tvoid *obj;\n> -\tint err;\n> -\n> -\terr = sctp_transport_walk_start(&hti);\n> -\tif (err)\n> -\t\treturn err;\n> +\tstruct sctp_transport *tsp;\n> +\tint ret;\n> \n> -\tobj = sctp_transport_get_idx(net, &hti, pos + 1);\n> -\tfor (; !IS_ERR_OR_NULL(obj); obj = sctp_transport_get_next(net, &hti)) {\n> -\t\tstruct sctp_transport *transport = obj;\n> +again:\n> +\tret = sctp_transport_walk_start(&hti);\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> -\t\tif (!sctp_transport_hold(transport))\n> +\ttsp = sctp_transport_get_idx(net, &hti, *pos + 1);\n> +\tfor (; !IS_ERR_OR_NULL(tsp); tsp = sctp_transport_get_next(net, &hti)) {\n> +\t\tif (!sctp_transport_hold(tsp))\n> \t\t\tcontinue;\n> -\t\terr = cb(transport, p);\n> -\t\tsctp_transport_put(transport);\n> -\t\tif (err)\n> +\t\tret = cb(tsp, p);\n> +\t\tif (ret)\n> \t\t\tbreak;\n> +\t\t(*pos)++;\n> +\t\tsctp_transport_put(tsp);\n> \t}\n> \tsctp_transport_walk_stop(&hti);\n> \n> -\treturn err;\n> +\tif (ret) {\n> +\t\tif (cb_done && !cb_done(tsp, p)) {\n> +\t\t\t(*pos)++;\n> +\t\t\tsctp_transport_put(tsp);\n> +\t\t\tgoto again;\n> +\t\t}\n> +\t\tsctp_transport_put(tsp);\n> +\t}\n> +\n> +\treturn ret;\n> }\n> EXPORT_SYMBOL_GPL(sctp_for_each_transport);\n> \n> -- \n> 2.1.0\n> \n> --\n> To unsubscribe from this list: send the line \"unsubscribe linux-sctp\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n>","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":["ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=fail (p=none dis=none) header.from=gmail.com","ext-mx04.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=marcelo.leitner@gmail.com"],"Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtwNX6Ghsz9s4s\n\tfor ;\n\tFri, 15 Sep 2017 22:55:08 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751476AbdIOMzG (ORCPT );\n\tFri, 15 Sep 2017 08:55:06 -0400","from mx1.redhat.com ([209.132.183.28]:49340 \"EHLO mx1.redhat.com\"\n\trhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP\n\tid S1751335AbdIOMzF (ORCPT );\n\tFri, 15 Sep 2017 08:55:05 -0400","from smtp.corp.redhat.com\n\t(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id 7824885542;\n\tFri, 15 Sep 2017 12:55:04 +0000 (UTC)","from localhost.localdomain (ovpn-116-12.gru2.redhat.com\n\t[10.97.116.12])\n\tby smtp.corp.redhat.com (Postfix) with SMTP id BFBE6757BC;\n\tFri, 15 Sep 2017 12:55:01 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com 7824885542","DKIM-Filter":"OpenDKIM Filter v2.11.0 mx1.redhat.com 7824885542","Date":"Fri, 15 Sep 2017 09:55:00 -0300","From":"Marcelo Ricardo Leitner ","To":"Xin Long ","Cc":"network dev , linux-sctp@vger.kernel.org,\n\tdavem@davemloft.net, Neil Horman ,\n\tpabeni@redhat.com","Subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","Message-ID":"<20170915125500.GC4529@localhost.localdomain>","References":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.14","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.28]);\n\tFri, 15 Sep 2017 12:55:04 +0000 (UTC)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1769169,"web_url":"http://patchwork.ozlabs.org/comment/1769169/","msgid":"<20170915125727.GA6031@hmswarspite.think-freely.org>","list_archive_url":null,"date":"2017-09-15T12:57:27","subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","submitter":{"id":411,"url":"http://patchwork.ozlabs.org/api/people/411/","name":"Neil Horman","email":"nhorman@tuxdriver.com"},"content":"On Fri, Sep 15, 2017 at 11:02:21AM +0800, Xin Long wrote:\n> Commit 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the\n> dump\") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep\n> with holding sock and sock lock.\n> \n> But Paolo noticed that endpoint could be destroyed in sctp_rcv without\n> sock lock protection. It means the use-after-free issue still could be\n> triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks\n> !ep, although it's pretty hard to reproduce.\n> \n> I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close\n> and sctp_sock_dump long time.\n> \n> This patch is to add another param cb_done to sctp_for_each_transport\n> and dump ep->assocs with holding tsp after jumping out of transport's\n> traversal in it to avoid this issue.\n> \n> It can also improve sctp diag dump to make it run faster, as no need\n> to save sk into cb->args[5] and keep calling sctp_for_each_transport\n> any more.\n> \n> This patch is also to use int * instead of int for the pos argument\n> in sctp_for_each_transport, which could make postion increment only\n> in sctp_for_each_transport and no need to keep changing cb->args[2]\n> in sctp_sock_filter and sctp_sock_dump any more.\n> \n> Fixes: 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the dump\")\n> Reported-by: Paolo Abeni \n> Signed-off-by: Xin Long \n> ---\n> include/net/sctp/sctp.h | 3 ++-\n> net/sctp/sctp_diag.c | 32 +++++++++-----------------------\n> net/sctp/socket.c | 40 +++++++++++++++++++++++++---------------\n> 3 files changed, 36 insertions(+), 39 deletions(-)\n> \nAcked-by: Neil Horman ","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xtwRh2rgXz9sRm\n\tfor ;\n\tFri, 15 Sep 2017 22:57:52 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751524AbdIOM5u (ORCPT );\n\tFri, 15 Sep 2017 08:57:50 -0400","from charlotte.tuxdriver.com ([70.61.120.58]:52586 \"EHLO\n\tsmtp.tuxdriver.com\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751207AbdIOM5s (ORCPT\n\t); Fri, 15 Sep 2017 08:57:48 -0400","from [2606:a000:111b:41f4:2201:d2d7:1fb:4ffb] (helo=localhost)\n\tby smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256)\n\t(Exim 4.63) (envelope-from )\n\tid 1dsqBU-0000Rp-CB; Fri, 15 Sep 2017 08:57:40 -0400"],"Date":"Fri, 15 Sep 2017 08:57:27 -0400","From":"Neil Horman ","To":"Xin Long ","Cc":"network dev , linux-sctp@vger.kernel.org,\n\tdavem@davemloft.net, Marcelo Ricardo Leitner ,\n\tpabeni@redhat.com","Subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","Message-ID":"<20170915125727.GA6031@hmswarspite.think-freely.org>","References":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","MIME-Version":"1.0","Content-Type":"text/plain; charset=us-ascii","Content-Disposition":"inline","In-Reply-To":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-Spam-Score":"-2.9 (--)","X-Spam-Status":"No","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}},{"id":1769474,"web_url":"http://patchwork.ozlabs.org/comment/1769474/","msgid":"<20170915.144844.2163359752292953670.davem@davemloft.net>","list_archive_url":null,"date":"2017-09-15T21:48:44","subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","submitter":{"id":15,"url":"http://patchwork.ozlabs.org/api/people/15/","name":"David Miller","email":"davem@davemloft.net"},"content":"From: Xin Long \nDate: Fri, 15 Sep 2017 11:02:21 +0800\n\n> Commit 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the\n> dump\") tried to fix an use-after-free issue by checking !sctp_sk(sk)->ep\n> with holding sock and sock lock.\n> \n> But Paolo noticed that endpoint could be destroyed in sctp_rcv without\n> sock lock protection. It means the use-after-free issue still could be\n> triggered when sctp_rcv put and destroy ep after sctp_sock_dump checks\n> !ep, although it's pretty hard to reproduce.\n> \n> I could reproduce it by mdelay in sctp_rcv while msleep in sctp_close\n> and sctp_sock_dump long time.\n> \n> This patch is to add another param cb_done to sctp_for_each_transport\n> and dump ep->assocs with holding tsp after jumping out of transport's\n> traversal in it to avoid this issue.\n> \n> It can also improve sctp diag dump to make it run faster, as no need\n> to save sk into cb->args[5] and keep calling sctp_for_each_transport\n> any more.\n> \n> This patch is also to use int * instead of int for the pos argument\n> in sctp_for_each_transport, which could make postion increment only\n> in sctp_for_each_transport and no need to keep changing cb->args[2]\n> in sctp_sock_filter and sctp_sock_dump any more.\n> \n> Fixes: 86fdb3448cc1 (\"sctp: ensure ep is not destroyed before doing the dump\")\n> Reported-by: Paolo Abeni \n> Signed-off-by: Xin Long \n\nApplied.","headers":{"Return-Path":"","X-Original-To":"patchwork-incoming@ozlabs.org","Delivered-To":"patchwork-incoming@ozlabs.org","Authentication-Results":"ozlabs.org;\n\tspf=none (mailfrom) smtp.mailfrom=vger.kernel.org\n\t(client-ip=209.132.180.67; helo=vger.kernel.org;\n\tenvelope-from=netdev-owner@vger.kernel.org;\n\treceiver=)","Received":["from vger.kernel.org (vger.kernel.org [209.132.180.67])\n\tby ozlabs.org (Postfix) with ESMTP id 3xv8DJ6dJTz9s3T\n\tfor ;\n\tSat, 16 Sep 2017 07:48:48 +1000 (AEST)","(majordomo@vger.kernel.org) by vger.kernel.org via listexpand\n\tid S1751537AbdIOVsq (ORCPT );\n\tFri, 15 Sep 2017 17:48:46 -0400","from shards.monkeyblade.net ([184.105.139.130]:57800 \"EHLO\n\tshards.monkeyblade.net\" rhost-flags-OK-OK-OK-OK) by vger.kernel.org\n\twith ESMTP id S1751413AbdIOVsp (ORCPT\n\t); Fri, 15 Sep 2017 17:48:45 -0400","from localhost (74-93-104-98-Washington.hfc.comcastbusiness.net\n\t[74.93.104.98]) (using TLSv1 with cipher AES256-SHA (256/256 bits))\n\t(Client did not present a certificate)\n\t(Authenticated sender: davem-davemloft)\n\tby shards.monkeyblade.net (Postfix) with ESMTPSA id 1D077133F6554;\n\tFri, 15 Sep 2017 14:48:45 -0700 (PDT)"],"Date":"Fri, 15 Sep 2017 14:48:44 -0700 (PDT)","Message-Id":"<20170915.144844.2163359752292953670.davem@davemloft.net>","To":"lucien.xin@gmail.com","Cc":"netdev@vger.kernel.org, linux-sctp@vger.kernel.org,\n\tmarcelo.leitner@gmail.com, nhorman@tuxdriver.com, pabeni@redhat.com","Subject":"Re: [PATCH net] sctp: fix an use-after-free issue in sctp_sock_dump","From":"David Miller ","In-Reply-To":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","References":"<9de7bef8f4f8c0f369361118f6d2daee5d188467.1505444541.git.lucien.xin@gmail.com>","X-Mailer":"Mew version 6.7 on Emacs 25.2 / Mule 6.0 (HANACHIRUSATO)","Mime-Version":"1.0","Content-Type":"Text/Plain; charset=us-ascii","Content-Transfer-Encoding":"7bit","X-Greylist":"Sender succeeded SMTP AUTH, not delayed by\n\tmilter-greylist-4.5.12 (shards.monkeyblade.net\n\t[149.20.54.216]); Fri, 15 Sep 2017 14:48:45 -0700 (PDT)","Sender":"netdev-owner@vger.kernel.org","Precedence":"bulk","List-ID":"","X-Mailing-List":"netdev@vger.kernel.org"}}]