netdev - Re: [PATCH] sctp: fix ASCONF list handling

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20150528101511.GA14938@hmsreliant.think-freely.org>
Date:	Thu, 28 May 2015 06:15:11 -0400
From:	Neil Horman <nhorman@...driver.com>
To:	mleitner@...hat.com
Cc:	netdev@...r.kernel.org, linux-sctp@...r.kernel.org,
	Daniel Borkmann <daniel@...earbox.net>,
	Vlad Yasevich <vyasevich@...il.com>,
	Michio Honda <micchie@....wide.ad.jp>
Subject: Re: [PATCH] sctp: fix ASCONF list handling

On Wed, May 27, 2015 at 09:52:17PM -0300, mleitner@...hat.com wrote:
> From: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> 
> ->auto_asconf_splist is per namespace and mangled by functions like
> sctp_setsockopt_auto_asconf() which doesn't guarantee any serialization.
> 
> Also, the call to inet_sk_copy_descendant() was backuping
> ->auto_asconf_list through the copy but was not honoring
> ->do_auto_asconf, which could lead to list corruption if it was
> different between both sockets.
> 
> This commit thus fixes the list handling by adding a spinlock to protect
> against multiple writers and converts the list to be protected by RCU
> too, so that we don't have a lock inverstion issue at
> sctp_addr_wq_timeout_handler().
> 
> And as this list now uses RCU, we cannot do such backup and restore
> while copying descendant data anymore as readers may be traversing the
> list meanwhile. We fix this by simply ignoring/not copying those fields,
> placed at the end of struct sctp_sock, so we can just ignore it together
> with struct ipv6_pinfo data. For that we create sctp_copy_descendant()
> so we don't clutter inet_sk_copy_descendant() with SCTP info.
> 
> Issue was found with a test application that kept flipping sysctl
> default_auto_asconf on and off.
> 
> Fixes: 9f7d653b67ae ("sctp: Add Auto-ASCONF support (core).")
> Signed-off-by: Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
> ---
>  include/net/netns/sctp.h   |  6 +++++-
>  include/net/sctp/structs.h |  2 ++
>  net/sctp/protocol.c        |  6 +++++-
>  net/sctp/socket.c          | 39 ++++++++++++++++++++++++++-------------
>  4 files changed, 38 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/netns/sctp.h b/include/net/netns/sctp.h
> index 3573a81815ad9e0efb6ceb721eb066d3726419f0..e080bebb3147af39c8275261f57018eb01e917b0 100644
> --- a/include/net/netns/sctp.h
> +++ b/include/net/netns/sctp.h
> @@ -30,12 +30,15 @@ struct netns_sctp {
>  	struct list_head local_addr_list;
>  	struct list_head addr_waitq;
>  	struct timer_list addr_wq_timer;
> -	struct list_head auto_asconf_splist;
> +	struct list_head __rcu auto_asconf_splist;
You should use the addr_wq_lock here instead of creating a new lock, as thats
already used to protect most accesses to the list you are concerned about.
Though truthfully, that shouldn't be necessecary.  The list in question is only
read in one location and only written in one location.  You can likely just
rcu-ify, as the write side is in process context and protected by lock_sock.

Neil

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html