| Message ID | 20201030012938.489557-1-jmaloy@redhat.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] tipc: add stricter control of reserved service types | expand |
| Context | Check | Description |
|---|---|---|
| jkicinski/cover_letter | success | Link |
| jkicinski/fixes_present | success | Link |
| jkicinski/patch_count | success | Link |
| jkicinski/tree_selection | success | Clearly marked for net-next |
| jkicinski/subject_prefix | success | Link |
| jkicinski/source_inline | success | Was 0 now: 0 |
| jkicinski/verify_signedoff | fail | Link |
| jkicinski/module_param | success | Was 0 now: 0 |
| jkicinski/build_32bit | success | Errors and warnings before: 114 this patch: 114 |
| jkicinski/kdoc | success | Errors and warnings before: 27 this patch: 27 |
| jkicinski/verify_fixes | success | Link |
| jkicinski/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 67 lines checked |
| jkicinski/build_allmodconfig_warn | success | Errors and warnings before: 113 this patch: 113 |
| jkicinski/header_inline | success | Link |
| jkicinski/stable | success | Stable not CCed |
On Thu, 29 Oct 2020 21:29:38 -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. > > Acked-by: Yung Xue <ying.xue@windriver.com> > > --- Please be careful with the separator, git am cuts off the rest of the message including your sign-off. > v2: Added warning printout when a user is blocked from binding, > as suggested by Jakub Kicinski > > Signed-off-by: Jon Maloy <jmaloy@redhat.com> Fixed up the message, and applied. Thanks!
diff --git a/net/tipc/socket.c b/net/tipc/socket.c index e795a8a2955b..69c4b16e8184 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,22 @@ 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) { + pr_warn_once("Can't bind to reserved service type %u\n", + addr->addr.nameseq.type); + 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);