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

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

On 10/10/20 10:56 PM, 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 types which were really in use
> ([0,1]), and block all the rest ([2,63]).
> 
> This is dangerous, 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., practically all 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 easily be filtered out, since it is the very first
> binding performed when the system is starting.
> 
> We can hence block TIPC_CFG_SRV completely, and only allow TIPC_TOP_SRV
> to be bound once, and the correct behavior is achieved. This is what we
> do in this commit.
> 
> 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 | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index e795a8a2955b..67875a5761d0 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -665,6 +665,7 @@ static int tipc_bind(struct socket *sock, struct sockaddr *uaddr,
>  	struct sockaddr_tipc *addr = (struct sockaddr_tipc *)uaddr;
>  	struct tipc_sock *tsk = tipc_sk(sk);
>  	int res = -EINVAL;
> +	u32 stype, dnode;
>  
>  	lock_sock(sk);
>  	if (unlikely(!uaddr_len)) {
> @@ -691,9 +692,10 @@ 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)) {
> +	stype = addr->addr.nameseq.type;
> +	if (stype < TIPC_RESERVED_TYPES &&
> +	    (stype != TIPC_TOP_SRV ||
> +	     tipc_nametbl_translate(sock_net(sk), stype, stype, &dnode))) {
>  		res = -EACCES;
>  		goto exit;
>  	}
>