All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS
@ 2026-05-04 10:28 Chuck Lever
  2026-05-04 10:28 ` [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED Chuck Lever
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Chuck Lever @ 2026-05-04 10:28 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman
  Cc: linux-nfs, netdev, Chuck Lever, Michael Nemanov

xs_tcp_tls_setup_socket() leaks the lower rpc_clnt when a signal
interrupts its TASK_KILLABLE wait for XPRT_LOCKED: the killed-wait
path jumps to out_unlock without calling rpc_shutdown_client(), so
the clnt and its xprt leak. Patch 1 calls rpc_shutdown_client()
before joining out_unlock.

Patch 2 fixes a use-after-free Michael Nemanov hit on an mTLS mount
whose client certificate the server rejected. Nothing pins the upper
rpc_clnt across the delayed connect_worker, so a fatal handshake
failure can let the mount caller free the clnt before
xs_tcp_tls_setup_socket() runs; the worker then dereferences freed
memory. A new rpc_hold_client() helper takes a reference for TLS
transports only and drops it on the worker's exit path.

Compile-tested only.

Recent related threads:

[1] https://lore.kernel.org/linux-nfs/20260309112041.1336519-1-bsdhenrymartin@gmail.com/T/#u

[2] https://lore.kernel.org/linux-nfs/a57879782d2d383e2d1af292fe2b9005a43ea06c.1773263233.git.bcodding@hammerspace.com/T/#u

---
Chuck Lever (2):
      SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED
      SUNRPC: pin upper rpc_clnt across the TLS connect_worker

 include/linux/sunrpc/clnt.h |  1 +
 net/sunrpc/clnt.c           | 19 +++++++++++++++++--
 net/sunrpc/xprtsock.c       | 16 ++++++++++++++--
 3 files changed, 32 insertions(+), 4 deletions(-)
---
base-commit: 22ca5f8e836e43f49c9b622f60e7ee48012a81c3
change-id: 20260504-sunrpc-tls-clnt-pin-c1c678775ade

Best regards,
--  
Chuck Lever <chuck.lever@oracle.com>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED
  2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
@ 2026-05-04 10:28 ` Chuck Lever
  2026-05-04 10:28 ` [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker Chuck Lever
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2026-05-04 10:28 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman
  Cc: linux-nfs, netdev, Chuck Lever

From: Chuck Lever <chuck.lever@oracle.com>

xs_tcp_tls_setup_socket() creates a temporary "lower" rpc_clnt with
rpc_create() to drive the inner TLS handshake, then waits for
XPRT_LOCKED on its xprt with TASK_KILLABLE so a stuck handshake can
be aborted by signal. When the wait is interrupted, the function
jumps to out_unlock without releasing lower_clnt. The success path
and the out_close error path both call
rpc_shutdown_client(lower_clnt); only the killed-wait path skips it,
leaking the clnt and its underlying xprt.

Call rpc_shutdown_client() on this path before joining out_unlock.
xprt_release_write() is not needed here because XPRT_LOCKED was
never acquired.

Fixes: 26e8bfa30dac ("SUNRPC/TLS: Lock the lower_xprt during the tls handshake")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtsock.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/net/sunrpc/xprtsock.c b/net/sunrpc/xprtsock.c
index 2e1fe6013361..3eccd4923e6c 100644
--- a/net/sunrpc/xprtsock.c
+++ b/net/sunrpc/xprtsock.c
@@ -2734,8 +2734,11 @@ static void xs_tcp_tls_setup_socket(struct work_struct *work)
 	lower_xprt = rcu_dereference(lower_clnt->cl_xprt);
 	rcu_read_unlock();
 
-	if (wait_on_bit_lock(&lower_xprt->state, XPRT_LOCKED, TASK_KILLABLE))
+	if (wait_on_bit_lock(&lower_xprt->state, XPRT_LOCKED, TASK_KILLABLE)) {
+		/* XPRT_LOCKED was never acquired. */
+		rpc_shutdown_client(lower_clnt);
 		goto out_unlock;
+	}
 
 	status = xs_tls_handshake_sync(lower_xprt, &upper_xprt->xprtsec);
 	if (status) {

-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker
  2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
  2026-05-04 10:28 ` [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED Chuck Lever
@ 2026-05-04 10:28 ` Chuck Lever
  2026-05-04 22:09 ` [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Michael Nemanov
  2026-06-24 16:52 ` Chuck Lever
  3 siblings, 0 replies; 6+ messages in thread
From: Chuck Lever @ 2026-05-04 10:28 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Simon Horman
  Cc: linux-nfs, netdev, Chuck Lever, Michael Nemanov

From: Chuck Lever <chuck.lever@oracle.com>

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>
---
 include/linux/sunrpc/clnt.h |  1 +
 net/sunrpc/clnt.c           | 19 +++++++++++++++++--
 net/sunrpc/xprtsock.c       | 11 ++++++++++-
 3 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/include/linux/sunrpc/clnt.h b/include/linux/sunrpc/clnt.h
index f8b406b0a1af..3c2b8c355ab3 100644
--- 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 bc8ca470718b..efa26899bc7d 100644
--- 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 3eccd4923e6c..359407aae03e 100644
--- 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);

-- 
2.53.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS
  2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
  2026-05-04 10:28 ` [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED Chuck Lever
  2026-05-04 10:28 ` [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker Chuck Lever
@ 2026-05-04 22:09 ` Michael Nemanov
  2026-06-24 16:52 ` Chuck Lever
  3 siblings, 0 replies; 6+ messages in thread
From: Michael Nemanov @ 2026-05-04 22:09 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust, Anna Schumaker, David S. Miller,
	Eric Dumazet, Jakub Kicinski, Paolo Abeni, Simon Horman
  Cc: linux-nfs, netdev, Chuck Lever



On 04/05/2026 13:28, Chuck Lever wrote:

> Patch 2 fixes a use-after-free Michael Nemanov hit on an mTLS mount
> whose client certificate the server rejected.

Reviewed and tested both patches. 
Confirmed 3-sec delayed work was happening multiple times without UAF.
Thank you.

Tested-by: Michael Nemanov <michael.nemanov@vastdata.com>
Reviewed-by: Michael Nemanov <michael.nemanov@vastdata.com>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS
  2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
                   ` (2 preceding siblings ...)
  2026-05-04 22:09 ` [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Michael Nemanov
@ 2026-06-24 16:52 ` Chuck Lever
  2026-06-24 17:41   ` Anna Schumaker
  3 siblings, 1 reply; 6+ messages in thread
From: Chuck Lever @ 2026-06-24 16:52 UTC (permalink / raw)
  To: Trond Myklebust, Anna Schumaker
  Cc: linux-nfs, netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Michael Nemanov, Chuck Lever

Gentle ping on this series, posted seven weeks ago. Michael
Nemanov reviewed and tested both patches the following day; he is
the reporter of the use-after-free that patch 2 addresses on an
mTLS mount whose client certificate the server rejected.

Could one of you queue these for an upcoming release? I am glad
to repost against a current base if that is easier to apply.

--
Chuck Lever

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS
  2026-06-24 16:52 ` Chuck Lever
@ 2026-06-24 17:41   ` Anna Schumaker
  0 siblings, 0 replies; 6+ messages in thread
From: Anna Schumaker @ 2026-06-24 17:41 UTC (permalink / raw)
  To: Chuck Lever, Trond Myklebust
  Cc: linux-nfs, netdev, David S. Miller, Eric Dumazet, Jakub Kicinski,
	Paolo Abeni, Simon Horman, Michael Nemanov, Chuck Lever

Hi Chuck,

On Wed, Jun 24, 2026, at 12:52 PM, Chuck Lever wrote:
> Gentle ping on this series, posted seven weeks ago. Michael
> Nemanov reviewed and tested both patches the following day; he is
> the reporter of the use-after-free that patch 2 addresses on an
> mTLS mount whose client certificate the server rejected.
>
> Could one of you queue these for an upcoming release? I am glad
> to repost against a current base if that is easier to apply.

I don't remember seeing these patches when they came in initially. Sorry
about that! I'll take a look soon, and try to include them in a bugfixes
pull request.

Anna

>
> --
> Chuck Lever

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2026-06-24 17:42 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-04 10:28 [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Chuck Lever
2026-05-04 10:28 ` [PATCH 1/2] SUNRPC: release lower rpc_clnt if killed waiting for XPRT_LOCKED Chuck Lever
2026-05-04 10:28 ` [PATCH 2/2] SUNRPC: pin upper rpc_clnt across the TLS connect_worker Chuck Lever
2026-05-04 22:09 ` [PATCH 0/2] Fix a few memory bugs in RPC-with-TLS Michael Nemanov
2026-06-24 16:52 ` Chuck Lever
2026-06-24 17:41   ` Anna Schumaker

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.