netdev - Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if sctp_bind_addr

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: <20190306182127.GB10683@neilslaptop.think-freely.org>
Date:   Wed, 6 Mar 2019 13:21:27 -0500
From:   Neil Horman <nhorman@...driver.com>
To:     Xin Long <lucien.xin@...il.com>
Cc:     network dev <netdev@...r.kernel.org>, linux-sctp@...r.kernel.org,
        davem@...emloft.net,
        Marcelo Ricardo Leitner <marcelo.leitner@...il.com>
Subject: Re: [PATCH net 1/3] sctp: sctp_sock_migrate() returns error if
 sctp_bind_addr_dup() fails

On Sun, Mar 03, 2019 at 05:54:53PM +0800, Xin Long wrote:
> It should fail to create the new sk if sctp_bind_addr_dup() fails
> when accepting or peeloff an association.
> 
> Signed-off-by: Xin Long <lucien.xin@...il.com>
> ---
>  net/sctp/socket.c | 34 ++++++++++++++++++++++++----------
>  1 file changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index a2771b3..22adb8d 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -102,9 +102,9 @@ static int sctp_send_asconf(struct sctp_association *asoc,
>  			    struct sctp_chunk *chunk);
>  static int sctp_do_bind(struct sock *, union sctp_addr *, int);
>  static int sctp_autobind(struct sock *sk);
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type);
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type);
>  
>  static unsigned long sctp_memory_pressure;
>  static atomic_long_t sctp_memory_allocated;
> @@ -4655,7 +4655,11 @@ static struct sock *sctp_accept(struct sock *sk, int flags, int *err, bool kern)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	error = sctp_sock_migrate(sk, newsk, asoc, SCTP_SOCKET_TCP);
> +	if (error) {
> +		sk_common_release(newsk);
sctp_sock_migrate may fail after the pending packets have been moved from the
old socket to the new socket.  Normally those packets will get purged by
successful transmission, or when the socket is closed (via sctp_close), but
neither of those cases applies here.  Whats going to dequeue and free any
pending skbs on the sk_receive_queue here?

> +		newsk = NULL;
> +	}
>  
>  out:
>  	release_sock(sk);
> @@ -5401,7 +5405,12 @@ int sctp_do_peeloff(struct sock *sk, sctp_assoc_t id, struct socket **sockp)
>  	/* Populate the fields of the newsk from the oldsk and migrate the
>  	 * asoc to the newsk.
>  	 */
> -	sctp_sock_migrate(sk, sock->sk, asoc, SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	err = sctp_sock_migrate(sk, sock->sk, asoc,
> +				SCTP_SOCKET_UDP_HIGH_BANDWIDTH);
> +	if (err) {
> +		sock_release(sock);
Same question here, what frees any pending skbs on the new socket, if the
migration fails after the skbs have been queued to it?

> +		sock = NULL;
> +	}
>  
>  	*sockp = sock;
>  
> @@ -8924,9 +8933,9 @@ static inline void sctp_copy_descendant(struct sock *sk_to,
>  /* Populate the fields of the newsk from the oldsk and migrate the assoc
>   * and its messages to the newsk.
>   */
> -static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> -			      struct sctp_association *assoc,
> -			      enum sctp_socket_type type)
> +static int sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
> +			     struct sctp_association *assoc,
> +			     enum sctp_socket_type type)
>  {
>  	struct sctp_sock *oldsp = sctp_sk(oldsk);
>  	struct sctp_sock *newsp = sctp_sk(newsk);
> @@ -8935,6 +8944,7 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	struct sk_buff *skb, *tmp;
>  	struct sctp_ulpevent *event;
>  	struct sctp_bind_hashbucket *head;
> +	int err;
>  
>  	/* Migrate socket buffer sizes and all the socket level options to the
>  	 * new socket.
> @@ -8963,8 +8973,10 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	/* Copy the bind_addr list from the original endpoint to the new
>  	 * endpoint so that we can handle restarts properly
>  	 */
> -	sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> -				&oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	err = sctp_bind_addr_dup(&newsp->ep->base.bind_addr,
> +				 &oldsp->ep->base.bind_addr, GFP_KERNEL);
> +	if (err)
> +		return err;
>  
>  	/* Move any messages in the old socket's receive queue that are for the
>  	 * peeled off association to the new socket's receive queue.
> @@ -9049,6 +9061,8 @@ static void sctp_sock_migrate(struct sock *oldsk, struct sock *newsk,
>  	}
>  
>  	release_sock(newsk);
> +
> +	return 0;
>  }
>  
>  
> -- 
> 2.1.0
> 
>