[PATCH net] rxrpc: Don't send on a service call before its connection is secured

[PATCH net] rxrpc: Don't send on a service call before its connection is secured

[Daniel Wade]

A service connection learns its security level only when the client's
RESPONSE has been verified; until then conn->security_level is 0 and
rxkad's cipher is NULL.  Since commit 41b996ce83bf ("rxrpc: Fix call state
set to not include the SERVER_SECURING state") a call leaves
SERVER_SECURING as soon as the request is queued, so rxrpc_send_data()
accepts a reply while the connection is still being challenged.

rxkad lays a DATA buffer out from the level at allocation but transforms
it from the level again at seal, and a partly filled buffer survives in
call->tx_pending across sendmsg() calls.  A buffer laid out at level 0
has no room for the security header, so if the RESPONSE raises the level
to 2 before the seal, rxkad_secure_packet_encrypt() pads and encrypts
round_up(8 + len, 8) bytes in place: 8 to 15 bytes past the end of the
page fragment, which are then transmitted.  rxgk splits the work the same
way and dereferences a NULL key context instead.

Refuse to queue data on a service call until its connection has reached
RXRPC_CONN_SERVICE, returning -EBUSY as rxrpc_do_sendmsg() already does
for a call still receiving its request.  The state is read under
conn->state_lock, so a sender that sees RXRPC_CONN_SERVICE also sees the
level and keys installed before it.

Discovered by an AI code review agent, reproduced on unmodified mainline
with KASAN: a server replying with MSG_MORE before the client's RESPONSE
was processed got every reply moved down by 8 bytes.  Same on v6.12.117.
With this patch the early sendmsg() calls return EBUSY and the replies
arrive intact.

Fixes: 41b996ce83bf ("rxrpc: Fix call state set to not include the SERVER_SECURING state")
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Wade <danjwade95@gmail.com>
---
Tested on mainline b3dfbde62cf1 in a 2-vCPU KVM guest with KASAN and
PROVE_LOCKING, AF_RXRPC and RXKAD built in, CONFIG_RXGK not set.  The
test runs as an unprivileged user and makes its client ticket with
AF_ALG ecb(des), so crypto.af_alg_restrict was set to 0 in the guest.
20 calls per kernel: unmodified, every reply arrived moved down by 8
bytes; with this patch, every reply arrived intact.  Same on v6.12.117.

KASAN stayed quiet throughout, as expected: page fragments are not
tracked per allocation, and here the 12-byte overrun falls into the 56
bytes of alignment padding behind the 1416-byte fragment.

Behaviour change: a server that sends on a charged user call ID before
it has been told about the call now gets EBUSY while the connection is
still being challenged.  If that sendmsg() also carried RXRPC_TX_LENGTH
the length has already been recorded, so a retry that passes it again
gets EINVAL.  Client calls and unsecured service connections are
unchanged.

Stable: trees without 7801c42c3bb7 ("rxrpc: Fix update of
call->tx_pending without holding lock") return directly from
rxrpc_send_data(), so there the new call site needs "return ret;"
instead of "goto out_unlock;".  6.6.y was not examined.

Not tested: rxgk, IPv6, interoperation with AFS servers or clients.  The
rxgk consequences described above come from reading net/rxrpc/rxgk.c and
crypto/krb5 only.

The test program is not included here; it is available on request.

 net/rxrpc/sendmsg.c |   37 +++++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)
diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
index 312be27ca75..a2f0bc19109 100644
--- a/net/rxrpc/sendmsg.c
+++ b/net/rxrpc/sendmsg.c
@@ -89,6 +89,31 @@ static int rxrpc_wait_to_be_connected(struct rxrpc_call *call, long *timeo)
 	return ret;
 }
 
+/*
+ * Check that a service call's connection is secured.  A DATA buffer's layout
+ * depends on the security level, which isn't known until the client's
+ * RESPONSE is verified.  The lock pairs with rxrpc_process_event() setting
+ * the level and keys before the state.
+ */
+static int rxrpc_check_service_secured(struct rxrpc_call *call)
+{
+	struct rxrpc_connection *conn = call->conn;
+	enum rxrpc_conn_proto_state state;
+
+	spin_lock_irq(&conn->state_lock);
+	state = conn->state;
+	spin_unlock_irq(&conn->state_lock);
+
+	switch (state) {
+	case RXRPC_CONN_SERVICE:
+		return 0;
+	case RXRPC_CONN_ABORTED:
+		return -ECONNABORTED;
+	default:
+		return -EBUSY;
+	}
+}
+
 /*
  * Return true if there's sufficient Tx queue space.
  */
@@ -349,6 +374,12 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
 			goto out_unlock;
 	}
 
+	if (rxrpc_is_service_call(call)) {
+		ret = rxrpc_check_service_secured(call);
+		if (ret < 0)
+			goto out_unlock;
+	}
+
 	/* this should be in poll */
 	sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);