[RDS/TCP,v1,1/1] RDS tcp loopback connection can hang

Commit Message

Shoaib Rao May 21, 2021, 6:08 p.m. UTC

From: Rao Shoaib <[e-mail]>

When TCP is used as transport and a program on the
system connects to RDS port 16385, connection is
accepted but denied per the rules of RDS. However,
RDS connections object is left in the list. Next
loopback connection will select that connection
object as it is at the head of list. The connection
attempt will hang as the connection object is set
to connect over TCP which is not allowed

The issue can be reproduced easily, use rds-ping
to ping a local IP address. After that use any
program like ncat to connect to the same IP
address and port 16385. This will hang so ctrl-c out.
Now try rds-ping, it will hang.

To fix the issue this patch adds checks to disallow
the connection object creation and destroys the
connection object.

Signed-off-by: Rao Shoaib <[e-mail]>
---
 net/rds/connection.c | 23 +++++++++++++++++------
 net/rds/tcp.c        |  4 ++--
 net/rds/tcp.h        |  3 ++-
 net/rds/tcp_listen.c |  6 ++++++
 4 files changed, 27 insertions(+), 9 deletions(-)

Comments

David Miller May 21, 2021, 9:25 p.m. UTC | #1

From: Rao Shoaib <[e-mail]>
Date: Fri, 21 May 2021 11:08:06 -0700

> +				/* No transport currently in use
> +				 * should end up here, but if it
> +				 * does, reset/destroy the connection.
> +				 */
> +				kmem_cache_free(rds_conn_slab, conn);
> +				conn = ERR_PTR(-EOPNOTSUPP);
> +				goto out;

Is thosa all we have to do?  What about releasing c_path[]?

Thanks.

Shoaib Rao May 21, 2021, 9:37 p.m. UTC | #2

On 5/21/21 2:25 PM, David Miller wrote:
> From: Rao Shoaib <[e-mail]>
> Date: Fri, 21 May 2021 11:08:06 -0700
>
>> +				/* No transport currently in use
>> +				 * should end up here, but if it
>> +				 * does, reset/destroy the connection.
>> +				 */
>> +				kmem_cache_free(rds_conn_slab, conn);
>> +				conn = ERR_PTR(-EOPNOTSUPP);
>> +				goto out;
> Is thosa all we have to do?  What about releasing c_path[]?
>
> Thanks.

rds_connection object is created before c_paths are populated. The code 
is killing the creation of rds_connection object, so there are no paths 
to free.

Thanks,

Shoaib

[e-mail] May 21, 2021, 9:50 p.m. UTC | #3

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Fri, 21 May 2021 11:08:06 -0700 you wrote:
> From: Rao Shoaib <[e-mail]>
> 
> When TCP is used as transport and a program on the
> system connects to RDS port 16385, connection is
> accepted but denied per the rules of RDS. However,
> RDS connections object is left in the list. Next
> loopback connection will select that connection
> object as it is at the head of list. The connection
> attempt will hang as the connection object is set
> to connect over TCP which is not allowed
> 
> [...]

Here is the summary with links:
  - [RDS/TCP,v1,1/1] RDS tcp loopback connection can hang
    https://git.kernel.org/netdev/net/c/aced3ce57cd3

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Patch

diff --git a/net/rds/connection.c b/net/rds/connection.c
index f2fcab182095..a3bc4b54d491 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -240,12 +240,23 @@  static struct rds_connection *__rds_conn_create(struct net *net,
 	if (loop_trans) {
 		rds_trans_put(loop_trans);
 		conn->c_loopback = 1;
-		if (is_outgoing && trans->t_prefer_loopback) {
-			/* "outgoing" connection - and the transport
-			 * says it wants the connection handled by the
-			 * loopback transport. This is what TCP does.
-			 */
-			trans = &rds_loop_transport;
+		if (trans->t_prefer_loopback) {
+			if (likely(is_outgoing)) {
+				/* "outgoing" connection to local address.
+				 * Protocol says it wants the connection
+				 * handled by the loopback transport.
+				 * This is what TCP does.
+				 */
+				trans = &rds_loop_transport;
+			} else {
+				/* No transport currently in use
+				 * should end up here, but if it
+				 * does, reset/destroy the connection.
+				 */
+				kmem_cache_free(rds_conn_slab, conn);
+				conn = ERR_PTR(-EOPNOTSUPP);
+				goto out;
+			}
 		}
 	}
 
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 43db0eca911f..abf19c0e3ba0 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -313,8 +313,8 @@  static void rds6_tcp_tc_info(struct socket *sock, unsigned int len,
 }
 #endif
 
-static int rds_tcp_laddr_check(struct net *net, const struct in6_addr *addr,
-			       __u32 scope_id)
+int rds_tcp_laddr_check(struct net *net, const struct in6_addr *addr,
+			__u32 scope_id)
 {
 	struct net_device *dev = NULL;
 #if IS_ENABLED(CONFIG_IPV6)
diff --git a/net/rds/tcp.h b/net/rds/tcp.h
index bad9cf49d565..dc8d745d6857 100644
--- a/net/rds/tcp.h
+++ b/net/rds/tcp.h
@@ -59,7 +59,8 @@  u32 rds_tcp_snd_una(struct rds_tcp_connection *tc);
 u64 rds_tcp_map_seq(struct rds_tcp_connection *tc, u32 seq);
 extern struct rds_transport rds_tcp_transport;
 void rds_tcp_accept_work(struct sock *sk);
-
+int rds_tcp_laddr_check(struct net *net, const struct in6_addr *addr,
+			__u32 scope_id);
 /* tcp_connect.c */
 int rds_tcp_conn_path_connect(struct rds_conn_path *cp);
 void rds_tcp_conn_path_shutdown(struct rds_conn_path *conn);
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 101cf14215a0..09cadd556d1e 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -167,6 +167,12 @@  int rds_tcp_accept_one(struct socket *sock)
 	}
 #endif
 
+	if (!rds_tcp_laddr_check(sock_net(sock->sk), peer_addr, dev_if)) {
+		/* local address connection is only allowed via loopback */
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
+
 	conn = rds_conn_create(sock_net(sock->sk),
 			       my_addr, peer_addr,
 			       &rds_tcp_transport, 0, GFP_KERNEL, dev_if);