SUNRPC: pin upper rpc_clnt across the TLS connect_worker

The TLS connect path has a use-after-free: nothing pins the
upper rpc_clnt across the delayed connect_worker. xs_connect()
stores task->tk_client in sock_xprt::clnt as a raw pointer
and queues the worker; for TLS-secured transports that worker
is xs_tcp_tls_setup_socket(), which reads several fields out
of the saved pointer (cl_timeout, cl_program, cl_prog,
cl_vers, cl_cred, cl_stats) to construct the args for the
inner handshake rpc_clnt.

The xprt does not reference the rpc_clnt; the rpc_clnt
references the xprt. xs_destroy() does cancel the
connect_worker, but it runs only when the xprt's refcount
drops to zero, which cannot happen until the rpc_clnt
releases its cl_xprt reference in rpc_free_client_work().
When a TLS handshake fails fatally (for example, an mTLS
mount whose client cert does not match the server), the
connecting task is woken with -EACCES and exits, the mount
caller invokes rpc_shutdown_client(), and the upper rpc_clnt
is freed before the queued connect_worker fires.
xs_tcp_tls_setup_socket() then dereferences the freed clnt,
producing the refcount_t underflow Michael Nemanov reported.

Take a reference on the upper rpc_clnt in xs_connect() for
TLS transports via a new rpc_hold_client() helper, and drop
it in the connect_worker's exit path with rpc_release_client().
The xprt_lock_connect() / xprt_unlock_connect() pairing
already serialises xs_connect() with xs_tcp_tls_setup_socket(),
so the take and release are balanced one-for-one.

The non-TLS connect worker (xs_tcp_setup_socket) never reads
sock_xprt::clnt, so leave that path alone and avoid the
clnt-holds-xprt-holds-clnt cycle that would otherwise prevent
xprt destruction.

Reported-by: Michael Nemanov <michael.nemanov@vastdata.com>
Closes: https://lore.kernel.org/linux-nfs/40e3d522-dfcf-4fc1-9c55-b5e81f1536d5@vastdata.com/
Fixes: 75eb6af7acdf ("SUNRPC: Add a TCP-with-TLS RPC transport class")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
Tested-by: Michael Nemanov <michael.nemanov@vastdata.com>
Reviewed-by: Michael Nemanov <michael.nemanov@vastdata.com>
Signed-off-by: Anna Schumaker <anna.schumaker@hammerspace.com>

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index f8b406b0a1af5d8bf7bcdfd0182c794ac09c3ab2..3c2b8c355ab3a85d4e4665dd073943466111b15f 100644 (file)
--- a/include/linux/sunrpc/clnt.h
+++ b/include/linux/sunrpc/clnt.h
@@ -190,6 +190,7 @@ int         rpc_switch_client_transport(struct rpc_clnt *,
                                const struct rpc_timeout *);
 
 void           rpc_shutdown_client(struct rpc_clnt *);
+void           rpc_hold_client(struct rpc_clnt *);
 void           rpc_release_client(struct rpc_clnt *);
 void           rpc_task_release_transport(struct rpc_task *);
 void           rpc_task_release_client(struct rpc_task *);

diff --git a/net/sunrpc/clnt.c b/net/sunrpc/clnt.c
index bc8ca470718b7ae3b7ecc2efc453c91c072f5dc6..efa26899bc7dcd97425d324d63ed7b55aed9c559 100644 (file)
--- a/net/sunrpc/clnt.c
+++ b/net/sunrpc/clnt.c
@@ -1026,8 +1026,23 @@ rpc_free_auth(struct rpc_clnt *clnt)
        return NULL;
 }
 
-/*
- * Release reference to the RPC client
+/**
+ * rpc_hold_client - acquire a reference on an rpc_clnt
+ * @clnt: rpc_clnt to pin
+ *
+ * Pairs with rpc_release_client().
+ */
+void rpc_hold_client(struct rpc_clnt *clnt)
+{
+       refcount_inc(&clnt->cl_count);
+}
+
+/**
+ * rpc_release_client - release a reference on an rpc_clnt
+ * @clnt: rpc_clnt to release
+ *
+ * Pairs with rpc_hold_client(). The rpc_clnt's resources are
+ * freed once its reference count drops to zero.
  */
 void
 rpc_release_client(struct rpc_clnt *clnt)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 3eccd4923e6c66f4a23ca0983b553831d633edc0..359407aae03e6dc64c06ef5de80368f69b5ee41a 100644 (file)
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2761,6 +2761,7 @@ static void xs_tcp_tls_setup_socket(struct work_struct *work)
 out_unlock:
        current_restore_flags(pflags, PF_MEMALLOC);
        upper_transport->clnt = NULL;
+       rpc_release_client(upper_clnt);
        xprt_unlock_connect(upper_xprt, upper_transport);
        return;
 
@@ -2808,7 +2809,15 @@ static void xs_connect(struct rpc_xprt *xprt, struct rpc_task *task)
        } else
                dprintk("RPC:       xs_connect scheduled xprt %p\n", xprt);
 
-       transport->clnt = task->tk_client;
+       /*
+        * Only the TLS connect_worker reads transport->clnt; pinning
+        * the upper rpc_clnt unconditionally would form a cycle with
+        * cl_xprt and prevent xprt destruction.
+        */
+       if (xprt->xprtsec.policy != RPC_XPRTSEC_NONE) {
+               rpc_hold_client(task->tk_client);
+               transport->clnt = task->tk_client;
+       }
        queue_delayed_work(xprtiod_workqueue,
                        &transport->connect_worker,
                        delay);