Re: [tipc-discussion] [net v3] tipc: add stricter control of reserved service types

[Ying X. <yin...@wi...>]

On 10/27/20 7:07 AM, jm...@re... wrote:
> From: Jon Maloy <jm...@re...>
> 
> TIPC reserves 64 service types for current and future internal use.
> Therefore, the bind() function is meant to block regular user sockets
> from being bound to these values, while it should let through such
> bindings from internal users.
> 
> However, since we at the design moment saw no way to distinguish
> between regular and internal users the filter function ended up
> with allowing all bindings of the reserved types which were really
> in use ([0,1]), and block all the rest ([2,63]).
> 
> This is risky, since a regular user may bind to the service type
> representing the topology server (TIPC_TOP_SRV == 1) or the one used
> for indicating neigboring node status (TIPC_CFG_SRV == 0), and wreak
> havoc for users of those services, i.e., most users.
> 
> The reality is however that TIPC_CFG_SRV never is bound through the
> bind() function, since it doesn't represent a regular socket, and
> TIPC_TOP_SRV can also be made to bypass the checks in tipc_bind()
> by introducing a different entry function, tipc_sk_bind().
> 
> It should be noted that, although this is a change of the API semantics,
> there is no risk we will break any currently working applications by
> doing this. Any application trying to bind to the values in question
> would be badly broken from the outset, so there is no chance we would
> find any such applications in real-world production systems.
> 
> Signed-off-by: Jon Maloy <jm...@re...>

Acked-by: Ying Xue <yin...@wi...>

> ---
>  net/tipc/socket.c | 24 +++++++++++++++---------
>  net/tipc/socket.h |  2 +-
>  net/tipc/topsrv.c |  4 ++--
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index e795a8a2955b..222fd53da2d0 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -658,8 +658,8 @@ static int tipc_release(struct socket *sock)
>   * NOTE: This routine doesn't need to take the socket lock since it doesn't
>   *       access any non-constant socket information.
>   */
> -static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
> -		     int uaddr_len)
> +
> +int tipc_sk_bind(struct socket *sock, struct sockaddr *uaddr, int uaddr_len)
>  {
>  	struct sock *sk = sock->sk;
>  	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
> @@ -691,13 +691,6 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
>  		goto exit;
>  	}
>  
> -	if ((addr->addr.nameseq.type < TIPC_RESERVED_TYPES) &&
> -	    (addr->addr.nameseq.type != TIPC_TOP_SRV) &&
> -	    (addr->addr.nameseq.type != TIPC_CFG_SRV)) {
> -		res = -EACCES;
> -		goto exit;
> -	}
> -
>  	res = (addr->scope >= 0) ?
>  		tipc_sk_publish(tsk, addr->scope, &addr->addr.nameseq) :
>  		tipc_sk_withdraw(tsk, -addr->scope, &addr->addr.nameseq);
> @@ -706,6 +699,19 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
>  	return res;
>  }
>  
> +static int tipc_bind(struct socket *sock, struct sockaddr *skaddr, int alen)
> +{
> +	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)skaddr;
> +
> +	if (alen) {
> +		if (alen < sizeof(struct sockaddr_tipc))
> +			return -EINVAL;
> +		if (addr->addr.nameseq.type < TIPC_RESERVED_TYPES)
> +			return -EACCES;
> +	}
> +	return tipc_sk_bind(sock, skaddr, alen);
> +}
> +
>  /**
>   * tipc_getname - get port ID of socket or peer socket
>   * @sock: socket structure
> diff --git a/net/tipc/socket.h b/net/tipc/socket.h
> index b11575afc66f..02cdf166807d 100644
> --- a/net/tipc/socket.h
> +++ b/net/tipc/socket.h
> @@ -74,7 +74,7 @@ int tipc_dump_done(struct netlink_callback *cb);
>  u32 tipc_sock_get_portid(struct sock *sk);
>  bool tipc_sk_overlimit1(struct sock *sk, struct sk_buff *skb);
>  bool tipc_sk_overlimit2(struct sock *sk, struct sk_buff *skb);
> -
> +int tipc_sk_bind(struct socket *sock, struct sockaddr *skaddr, int alen);
>  int tsk_set_importance(struct sock *sk, int imp);
>  
>  #endif
> diff --git a/net/tipc/topsrv.c b/net/tipc/topsrv.c
> index 5f6f86051c83..cec029349662 100644
> --- a/net/tipc/topsrv.c
> +++ b/net/tipc/topsrv.c
> @@ -520,12 +520,12 @@ static int tipc_topsrv_create_listener(struct tipc_topsrv *srv)
>  
>  	saddr.family	                = AF_TIPC;
>  	saddr.addrtype		        = TIPC_ADDR_NAMESEQ;
> -	saddr.addr.nameseq.type	        = TIPC_TOP_SRV;
> +	saddr.addr.nameseq.type         = TIPC_TOP_SRV;
>  	saddr.addr.nameseq.lower	= TIPC_TOP_SRV;
>  	saddr.addr.nameseq.upper	= TIPC_TOP_SRV;
>  	saddr.scope			= TIPC_NODE_SCOPE;
>  
> -	rc = kernel_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
> +	rc = tipc_sk_bind(lsock, (struct sockaddr *)&saddr, sizeof(saddr));
>  	if (rc < 0)
>  		goto err;
>  	rc = kernel_listen(lsock, 0);
>