[PATCH] ksmbd: fix smb2_cancel() use-after-free during request decompression

[Ian Bridges <icb@fastmail.org>]

smb2_cancel() finds the request to cancel by reading the SMB2 header in
the request_buf of each queued struct ksmbd_work. A queued request may
still be in preprocessing. smb3_decrypt_req() decrypts it in place, and
ksmbd_decompress_work_request() frees request_buf and replaces it with
the decompressed buffer. Neither runs under conn->request_lock. A
concurrent CANCEL can therefore compare a transform header as if it were
an SMB2 header, or read freed memory.

  BUG: KASAN: slab-use-after-free in smb2_cancel+0x73b/0x840 [ksmbd]
  Call Trace:
   smb2_cancel+0x73b/0x840 [ksmbd]
   handle_ksmbd_work+0xa90/0x2110 [ksmbd]
  Freed by task 53136:
   ksmbd_decompress_work_request+0x10d/0x180 [ksmbd]

Record the MessageId and Command of each request in its struct
ksmbd_work, under conn->request_lock. A plain SMB2 request gets them
when it is queued, because its request_buf does not change. An encrypted
request gets them once it is decrypted and decompressed. smb2_cancel()
matches on these fields and skips a request that does not have them yet.
It no longer reads the request_buf of another request.

Fixes: 6639b6928ba2 ("ksmbd: handle encrypted compressed requests")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---

Notes:
    The bug needs an authenticated client on an SMB 3.1.1 connection with
    encryption and compression negotiated. The client sends an encrypted,
    compressed request. An SMB2_CANCEL on the same connection is then
    processed while that request is being decompressed.
    
    A reproducer is available on request.
    
    Tested on ksmbd-for-next dc6eabb1bd40 under KASAN. The race window was
    widened with a temporary msleep() in ksmbd_decompress_work_request(),
    not part of this patch. A CANCEL sent during decompression no longer
    triggers the report. A sync CANCEL still finds an in-flight request by
    its MessageId, and an async CANCEL of a pending blocking lock still
    returns STATUS_CANCELLED. smbtorture results are unchanged.
    
    This fix was developed with LLM assistance.

 fs/smb/server/connection.c | 27 +++++++++++++++++++++++++++
 fs/smb/server/connection.h |  1 +
 fs/smb/server/ksmbd_work.h |  7 +++++++
 fs/smb/server/server.c     |  2 ++
 fs/smb/server/smb2pdu.c    | 18 +++++++++---------
 5 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/fs/smb/server/connection.c b/fs/smb/server/connection.c
index 0fcf90b877ae..01e5cc13a12a 100644
--- a/fs/smb/server/connection.c
+++ b/fs/smb/server/connection.c
@@ -352,6 +352,32 @@ bool ksmbd_conn_lookup_dialect(struct ksmbd_conn *c)
 	return ret;
 }
 
+/* Caller holds conn->request_lock */
+static void __ksmbd_conn_set_request_id(struct ksmbd_work *work)
+{
+	struct smb2_hdr *hdr = smb_get_msg(work->request_buf);
+
+	if (hdr->ProtocolId != SMB2_PROTO_NUMBER)
+		return;
+
+	work->req_mid = hdr->MessageId;
+	work->req_cmd = hdr->Command;
+	work->req_id_valid = true;
+}
+
+/*
+ * Record the MessageId and Command of a request whose request_buf now holds
+ * a plain SMB2 header, after decryption and decompression replaced it
+ */
+void ksmbd_conn_set_request_id(struct ksmbd_work *work)
+{
+	struct ksmbd_conn *conn = work->conn;
+
+	spin_lock(&conn->request_lock);
+	__ksmbd_conn_set_request_id(work);
+	spin_unlock(&conn->request_lock);
+}
+
 void ksmbd_conn_enqueue_request(struct ksmbd_work *work)
 {
 	struct ksmbd_conn *conn = work->conn;
@@ -363,6 +389,7 @@ void ksmbd_conn_enqueue_request(struct ksmbd_work *work)
 	atomic_inc(&conn->req_running);
 	if (requests_queue) {
 		spin_lock(&conn->request_lock);
+		__ksmbd_conn_set_request_id(work);
 		list_add_tail(&work->request_entry, requests_queue);
 		spin_unlock(&conn->request_lock);
 	}
diff --git a/fs/smb/server/connection.h b/fs/smb/server/connection.h
index e202bf7f3b98..6855c1ab691e 100644
--- a/fs/smb/server/connection.h
+++ b/fs/smb/server/connection.h
@@ -242,6 +242,7 @@ int ksmbd_conn_rdma_write(struct ksmbd_conn *conn,
 			  void *buf, unsigned int buflen,
 			  struct smbdirect_buffer_descriptor_v1 *desc,
 			  unsigned int desc_len);
+void ksmbd_conn_set_request_id(struct ksmbd_work *work);
 void ksmbd_conn_enqueue_request(struct ksmbd_work *work);
 void ksmbd_conn_try_dequeue_request(struct ksmbd_work *work);
 void ksmbd_conn_init_server_callbacks(struct ksmbd_conn_ops *ops);
diff --git a/fs/smb/server/ksmbd_work.h b/fs/smb/server/ksmbd_work.h
index 1ab9ddd20506..27c8705910fe 100644
--- a/fs/smb/server/ksmbd_work.h
+++ b/fs/smb/server/ksmbd_work.h
@@ -36,6 +36,13 @@ struct ksmbd_work {
 
 	/* Pointer to received SMB header */
 	void                            *request_buf;
+	/*
+	 * MessageId and Command of the request, set under conn->request_lock
+	 * once request_buf holds a plain SMB2 header, for smb2_cancel()
+	 */
+	__le64                          req_mid;
+	__le16                          req_cmd;
+	bool                            req_id_valid;
 	/* Response buffer */
 	void                            *response_buf;
 
diff --git a/fs/smb/server/server.c b/fs/smb/server/server.c
index e91e3a1459e5..962f12e969ba 100644
--- a/fs/smb/server/server.c
+++ b/fs/smb/server/server.c
@@ -295,6 +295,8 @@ static void __handle_ksmbd_work(struct ksmbd_work *work,
 			ksmbd_conn_abort(conn);
 			return;
 		}
+
+		ksmbd_conn_set_request_id(work);
 	}
 
 	if (conn->ops->allocate_rsp_buf(work))
diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index d8da0b259cfd..dda5f4ddbb43 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -10698,7 +10698,6 @@ int smb2_cancel(struct ksmbd_work *work)
 {
 	struct ksmbd_conn *conn = work->conn;
 	struct smb2_hdr *hdr = smb_get_msg(work->request_buf);
-	struct smb2_hdr *chdr;
 	struct ksmbd_work *iter;
 	struct list_head *command_list;
 
@@ -10715,8 +10714,6 @@ int smb2_cancel(struct ksmbd_work *work)
 		spin_lock(&conn->request_lock);
 		list_for_each_entry(iter, command_list,
 				    async_request_entry) {
-			chdr = smb_get_msg(iter->request_buf);
-
 			if (iter->async_id !=
 			    le64_to_cpu(hdr->Id.AsyncId))
 				continue;
@@ -10736,7 +10733,7 @@ int smb2_cancel(struct ksmbd_work *work)
 			ksmbd_debug(SMB,
 				    "smb2 with AsyncId %llu cancelled command = 0x%x\n",
 				    le64_to_cpu(hdr->Id.AsyncId),
-				    le16_to_cpu(chdr->Command));
+				    le16_to_cpu(iter->req_cmd));
 			if (iter->cancel_fn)
 				iter->cancel_fn(iter->cancel_argv);
 			break;
@@ -10747,10 +10744,13 @@ int smb2_cancel(struct ksmbd_work *work)
 
 		spin_lock(&conn->request_lock);
 		list_for_each_entry(iter, command_list, request_entry) {
-			chdr = smb_get_msg(iter->request_buf);
-
-			if (chdr->MessageId != hdr->MessageId ||
-			    iter == work)
+			/*
+			 * request_buf of another work may be replaced while
+			 * it is decrypted or decompressed, so match on the
+			 * MessageId recorded under request_lock instead.
+			 */
+			if (iter == work || !iter->req_id_valid ||
+			    iter->req_mid != hdr->MessageId)
 				continue;
 
 			if (cmpxchg(&iter->state, KSMBD_WORK_ACTIVE,
@@ -10760,7 +10760,7 @@ int smb2_cancel(struct ksmbd_work *work)
 			ksmbd_debug(SMB,
 				    "smb2 with mid %llu cancelled command = 0x%x\n",
 				    le64_to_cpu(hdr->MessageId),
-				    le16_to_cpu(chdr->Command));
+				    le16_to_cpu(iter->req_cmd));
 			if (iter->cancel_fn)
 				iter->cancel_fn(iter->cancel_argv);
 			break;

base-commit: dc6eabb1bd40c0655837844c58432ae9095f6979
-- 
2.47.3