[PATCH net-next] net/smc: abort the connection when the peer overruns the RMB

Hidayath Khan <[email protected]>
Newsgroups org.kernel.vger.linux-s390,org.kernel.vger.netdev
Message-ID <[email protected]>
smc_cdc_msg_recv_action() accumulates the peer's producer cursor advance
into conn->bytes_to_rcv:

        atomic_add(diff_prod, &conn->bytes_to_rcv);
        /* guarantee 0 <= bytes_to_rcv <= rmb_desc->len */

Nothing enforces that comment.  A conforming peer cannot advance its
producer cursor past our consumer cursor, so a peer that does fills the
accumulator without bound.

Bounding the cursor does not help: a wrap increment with count 0 is a legal
advance of exactly one bufferful, so every per-cursor check accepts it
while smc_curs_diff() returns size for each such message.

The copy-length clamps bound the copy but never repair the counter, so
SIOCINQ reports a readable length that does not exist and
smc_rx_data_available() keeps poll() readable forever with nothing for
recvmsg() to return.

Drop the connection instead, reusing the abort smc_cdc_msg_validate()
performs for an out-of-range sequence number.  The test precedes the
atomic_add() so the counter is never left inflated; abandoning the rest of
the message matches smc_cdc_msg_validate(), and abort_work tears the
connection down.

The test is written as a subtraction.  diff_prod is not bounded by the RMB:
smc_curs_diff() documents "difference cannot exceed size" as an assumption
about its inputs, but the peer controls the cursor it is computed from, so
it can return up to INT_MAX.  Adding that to bytes_to_rcv would overflow
and the comparison would then read as false, defeating the check in exactly
the case it exists for.  Subtracting cannot overflow, since bytes_to_rcv is
in [0, rmb_desc->len].

Both transports are affected: the accumulator is shared.

Reviewed-by: Sidraya Jayagond <[email protected]>
Reviewed-by: Mahanta Jambigi <[email protected]>
Signed-off-by: Hidayath Khan <[email protected]>
---
 net/smc/smc_cdc.c | 39 ++++++++++++++++++++++++++++-----------
 1 file changed, 28 insertions(+), 11 deletions(-)

diff --git a/net/smc/smc_cdc.c b/net/smc/smc_cdc.c
index 32d6d03df321..d8f747bbc6f6 100644
--- a/net/smc/smc_cdc.c
+++ b/net/smc/smc_cdc.c
@@ -305,6 +305,27 @@ static void smc_cdc_handle_urg_data_arrival(struct smc_sock *smc,
 	sk_send_sigurg(&smc->sk);
 }
 
+/* Drop a connection whose peer violated the CDC protocol.  @link is the link
+ * the offending message arrived on, or NULL when it is not known to the caller
+ * (SMC-D, and the shared receive path).  When @link is NULL conn->lnk is left
+ * unchanged: for SMC-R it already points to the right QP; for SMC-D there is
+ * no per-link QP to abort on.
+ */
+static void smc_cdc_abort_conn(struct smc_sock *smc, struct smc_link *link)
+{
+	struct smc_connection *conn = &smc->conn;
+
+	conn->out_of_sync = 1;  /* prevent any further receives */
+	spin_lock_bh(&conn->send_lock);
+	conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
+	if (link)
+		conn->lnk = link;
+	spin_unlock_bh(&conn->send_lock);
+	sock_hold(&smc->sk); /* sock_put in abort_work */
+	if (!queue_work(smc_close_wq, &conn->abort_work))
+		sock_put(&smc->sk);
+}
+
 static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 				 struct smc_link *link)
 {
@@ -314,17 +335,8 @@ static void smc_cdc_msg_validate(struct smc_sock *smc, struct smc_cdc_msg *cdc,
 
 	/* check that seqnum was seen before */
 	diff = conn->local_rx_ctrl.seqno - recv_seq;
-	if (diff < 0) { /* diff larger than 0x7fff */
-		/* drop connection */
-		conn->out_of_sync = 1;	/* prevent any further receives */
-		spin_lock_bh(&conn->send_lock);
-		conn->local_tx_ctrl.conn_state_flags.peer_conn_abort = 1;
-		conn->lnk = link;
-		spin_unlock_bh(&conn->send_lock);
-		sock_hold(&smc->sk); /* sock_put in abort_work */
-		if (!queue_work(smc_close_wq, &conn->abort_work))
-			sock_put(&smc->sk);
-	}
+	if (diff < 0) /* diff larger than 0x7fff */
+		smc_cdc_abort_conn(smc, link);  /* drop connection */
 }
 
 static void smc_cdc_msg_recv_action(struct smc_sock *smc,
@@ -376,6 +388,11 @@ static void smc_cdc_msg_recv_action(struct smc_sock *smc,
 	diff_prod = smc_curs_diff(conn->rmb_desc->len, &prod_old,
 				  &conn->local_rx_ctrl.prod);
 	if (diff_prod) {
+		if (diff_prod > conn->rmb_desc->len -
+				atomic_read(&conn->bytes_to_rcv)) {
+			smc_cdc_abort_conn(smc, NULL);
+			return;
+		}
 		if (conn->local_rx_ctrl.prod_flags.urg_data_present)
 			smc_cdc_handle_urg_data_arrival(smc, &diff_prod);
 		/* bytes_to_rcv is decreased in smc_recvmsg */

base-commit: cd138abf911f7a6b19656e446b09cff4453d1f3d
-- 
2.52.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.