[PATCH net] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[PATCH net] net/smc: fix use-after-free in smc_rx_pipe_buf_release()

[Hidayath Khan]

smc_rx_splice() hands RMB pages to a pipe and takes a socket reference for
each entry, so the smc_sock survives until the reader is done.  The
connection does not: a close in between runs smc_conn_free(), which
releases the link group and returns the receive buffer to the link group's
pool.

smc_rx_pipe_buf_release() tries to detect that by testing sk_state, but it
does so before taking the socket lock, and then dereferences the connection
anyway:

        if (sk->sk_state == SMC_CLOSED || ...)
                goto out;
        conn = &smc->conn;
        lock_sock(sk);
        smc_rx_update_cons(smc, priv->len);

smc_rx_update_cons() reads conn->rmb_desc->len twice and then calls
smc_tx_consumer_update(), which walks conn->lgr and conn->lnk.  The state
can change between the test and the lock, and on the is_reg_err path
smcr_buf_unuse() does not recycle the descriptor but frees it outright, so
this is a use-after-free rather than a stale read.

sk_state is also the wrong thing to test.  Take the socket lock first so
the test and the cursor update cannot be separated, and test the receive
buffer itself, which is what the code goes on to dereference.

For that test to mean anything, smc_buf_unuse() has to stop leaving a
pointer to a descriptor it has just released; clear conn->rmb_desc there.
Nothing in smc_conn_free() reads it afterwards, and smc_ism_unset_conn()
already returns early on a NULL rmb_desc, so an SMC-D teardown that reaches
it twice becomes a no-op instead of indexing smcd->conn[] with a stale
sba_idx.

Fixes: 9014db202cb7 ("smc: add support for splice()")
Cc: stable@vger.kernel.org
Reviewed-by: Sidraya Jayagond <sidraya@linux.ibm.com>
Signed-off-by: Hidayath Khan <hidayath@linux.ibm.com>
---
 net/smc/smc_core.c |  1 +
 net/smc/smc_rx.c   | 10 ++++++----
 2 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
index c0027d2fe4e8..def65ebc0b53 100644
--- a/net/smc/smc_core.c
+++ b/net/smc/smc_core.c
@@ -1248,6 +1248,7 @@ static void smc_buf_unuse(struct smc_connection *conn,
 			WRITE_ONCE(conn->rmb_desc->used, 0);
 		}
 		SMC_STAT_RMB_SIZE(smc, is_smcd, true, false, bufsize);
+		conn->rmb_desc = NULL;
 	}
 }
 
diff --git a/net/smc/smc_rx.c b/net/smc/smc_rx.c
index c1d9b923938d..c17d4757ec84 100644
--- a/net/smc/smc_rx.c
+++ b/net/smc/smc_rx.c
@@ -116,15 +116,17 @@ static void smc_rx_pipe_buf_release(struct pipe_inode_info *pipe,
 {
 	struct smc_spd_priv *priv = (struct smc_spd_priv *)buf->private;
 	struct smc_sock *smc = priv->smc;
-	struct smc_connection *conn;
+	struct smc_connection *conn = &smc->conn;
 	struct sock *sk = &smc->sk;
 
+	lock_sock(sk);
 	if (sk->sk_state == SMC_CLOSED ||
 	    sk->sk_state == SMC_PEERFINCLOSEWAIT ||
-	    sk->sk_state == SMC_APPFINCLOSEWAIT)
+	    sk->sk_state == SMC_APPFINCLOSEWAIT ||
+	    !conn->rmb_desc) {
+		release_sock(sk);
 		goto out;
-	conn = &smc->conn;
-	lock_sock(sk);
+	}
 	smc_rx_update_cons(smc, priv->len);
 	release_sock(sk);
 	if (atomic_sub_and_test(priv->len, &conn->splice_pending))

base-commit: 1c2c67f1a9009f643d1233fa6bb8f888deb797ba
-- 
2.52.0