| Message ID | 20201028131912.3773561-1-jmaloy@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] tipc: add stricter control of reserved service types | expand |
On Wed, 28 Oct 2020 09:19:12 -0400 jmaloy@redhat.com wrote: > From: Jon Maloy <jmaloy@redhat.com> > > 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 neighboring 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. You say above that it would wreak havoc for most users, not all :) I'd be more comfortable applying this to net-next, does that work for you? Also perhaps we could add a pr_warn_once() if an application tried using the reserved values, to help identify this change right away. > Acked-by: Yung Xue <ying.xue@windriver.com> > Signed-off-by: Jon Maloy <jmaloy@redhat.com>
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);