From patchwork Mon Nov 27 22:50:04 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Cengiz Can X-Patchwork-Id: 1869040 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4SfLRb046Dz1yRy for ; Tue, 28 Nov 2023 09:51:22 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1r7kRV-00050i-U6; Mon, 27 Nov 2023 22:51:08 +0000 Received: from smtp-relay-internal-1.internal ([10.131.114.114] helo=smtp-relay-internal-1.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1r7kRA-0004sw-FO for kernel-team@lists.ubuntu.com; Mon, 27 Nov 2023 22:50:49 +0000 Received: from mail-pf1-f197.google.com (mail-pf1-f197.google.com [209.85.210.197]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id 75A1E3F68B for ; Mon, 27 Nov 2023 22:50:41 +0000 (UTC) Received: by mail-pf1-f197.google.com with SMTP id d2e1a72fcca58-6cd976b287aso1303439b3a.1 for ; Mon, 27 Nov 2023 14:50:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701125439; x=1701730239; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=QzlF3QgGEnqSN8wInaxnSfJRZa7Op1iXBgpc/AEYo0o=; b=d9n76rRgGI/ICR64bFUAfXe4RbUlvy5mjsabPXsHbCz20rBgv4a7CvwxyeFM5z/pOx 9NXJX05OUCXCK8HQEQZQwLFjOU56PDKeDPHlkwZo8BBl9t4Xe3oJ5zONo9wPMr1xRqpt nf72Ej0p6uoCyGZl3DWP0+LilZZVNf19rSrYN4hrUVCCaObgi3AQGWO9H1ouVyFYXQI5 70cOaBDlqn+L+hWDreAyTLnt+qNPCVkmK/ISJhXuxVY7qAWga/NHl0DOEpYRNkDAYuzB VMdHRNeaxnsKJrn0hkDI/hgt9F69nNJMlQa7JRuFrwGW7s9UdJ8TgAwZvxjVrvJ0/0Ck OxIA== X-Gm-Message-State: AOJu0YzeeMRgpiuc2+DB6MyquX5RUKBHV/4nffzgRE5YXGVHqHY57sdD rGkvtJOZaHLebNZQ00V7zb2PLAOrvmF4e4JwmJhF9lIzS3y49EerOJSxTa+P2ZH20xsrYbjvRmB D3nn0SKDPMct6MvDWUq2e1kOsNJ5wix3rRSyaUwzcMSio55iyqXfO X-Received: by 2002:a05:6a20:7486:b0:18c:5795:669e with SMTP id p6-20020a056a20748600b0018c5795669emr7477618pzd.19.1701125439104; Mon, 27 Nov 2023 14:50:39 -0800 (PST) X-Google-Smtp-Source: AGHT+IGuIEytJPq6OgF4ABx76S/XGaa6DqPMaFiUee/tj84raaSDbTI676XMAHxG7kH85Q01lXG16g== X-Received: by 2002:a05:6a20:7486:b0:18c:5795:669e with SMTP id p6-20020a056a20748600b0018c5795669emr7477601pzd.19.1701125438620; Mon, 27 Nov 2023 14:50:38 -0800 (PST) Received: from localhost (uk.sesame.canonical.com. [185.125.190.60]) by smtp.gmail.com with ESMTPSA id z7-20020aa785c7000000b00688965c5227sm7613367pfn.120.2023.11.27.14.50.37 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 27 Nov 2023 14:50:38 -0800 (PST) From: Cengiz Can To: kernel-team@lists.ubuntu.com Subject: [SRU Jammy 3/3] ksmbd: fix racy issue from session setup and logoff Date: Tue, 28 Nov 2023 01:50:04 +0300 Message-Id: <20231127225004.615336-4-cengiz.can@canonical.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20231127225004.615336-1-cengiz.can@canonical.com> References: <20231127225004.615336-1-cengiz.can@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Namjae Jeon This racy issue is triggered by sending concurrent session setup and logoff requests. This patch does not set connection status as KSMBD_SESS_GOOD if state is KSMBD_SESS_NEED_RECONNECT in session setup. And relookup session to validate if session is deleted in logoff. Cc: stable@vger.kernel.org Reported-by: zdi-disclosures@trendmicro.com # ZDI-CAN-20481, ZDI-CAN-20590, ZDI-CAN-20596 Signed-off-by: Namjae Jeon Signed-off-by: Steve French CVE-2023-32252 (backported from commit f5c779b7ddbda30866cf2a27c63e34158f858c73) [cengizcan: we have commit 5005bcb42191 ("ksmbd: validate session id and tree id in the compound request") but due to missing commit e2f34481b24d ("cifsd: add server-side procedures for SMB3"), returned error values differ from upstream. opt in to use -EINVAL in `smb2_check_user_session` just like upstream does] [cengizcan: we don't have commit cb4517201b8a ("ksmbd: remove smb2_buf_length in smb2_hdr") (it was introduced in 5.16 and needed a number of fixes anyway). so instead of introducing lines that call `smb2_get_msg()`, keep direct `work->` accesses] Signed-off-by: Cengiz Can --- fs/ksmbd/connection.c | 14 ++++---- fs/ksmbd/connection.h | 39 +++++++++++--------- fs/ksmbd/mgmt/user_session.c | 1 + fs/ksmbd/server.c | 3 +- fs/ksmbd/smb2pdu.c | 69 +++++++++++++++++++++++------------- fs/ksmbd/transport_tcp.c | 2 +- 6 files changed, 78 insertions(+), 50 deletions(-) diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c index 8e2500a5fa01..3c4b7a96919c 100644 --- a/fs/ksmbd/connection.c +++ b/fs/ksmbd/connection.c @@ -56,7 +56,7 @@ struct ksmbd_conn *ksmbd_conn_alloc(void) return NULL; conn->need_neg = true; - conn->status = KSMBD_SESS_NEW; + ksmbd_conn_set_new(conn); conn->local_nls = load_nls("utf8"); if (!conn->local_nls) conn->local_nls = load_nls_default(); @@ -142,12 +142,12 @@ int ksmbd_conn_try_dequeue_request(struct ksmbd_work *work) return ret; } -static void ksmbd_conn_lock(struct ksmbd_conn *conn) +void ksmbd_conn_lock(struct ksmbd_conn *conn) { mutex_lock(&conn->srv_mutex); } -static void ksmbd_conn_unlock(struct ksmbd_conn *conn) +void ksmbd_conn_unlock(struct ksmbd_conn *conn) { mutex_unlock(&conn->srv_mutex); } @@ -240,7 +240,7 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) if (!ksmbd_server_running()) return false; - if (conn->status == KSMBD_SESS_EXITING) + if (ksmbd_conn_exiting(conn)) return false; if (kthread_should_stop()) @@ -300,7 +300,7 @@ int ksmbd_conn_handler_loop(void *p) pdu_size = get_rfc1002_len(hdr_buf); ksmbd_debug(CONN, "RFC1002 header %u bytes\n", pdu_size); - if (conn->status == KSMBD_SESS_GOOD) + if (ksmbd_conn_good(conn)) max_allowed_pdu_size = SMB3_MAX_MSGSIZE + conn->vals->max_write_size; else @@ -309,7 +309,7 @@ int ksmbd_conn_handler_loop(void *p) if (pdu_size > max_allowed_pdu_size) { pr_err_ratelimited("PDU length(%u) excceed maximum allowed pdu size(%u) on connection(%d)\n", pdu_size, max_allowed_pdu_size, - conn->status); + READ_ONCE(conn->status)); break; } @@ -412,7 +412,7 @@ static void stop_sessions(void) if (task) ksmbd_debug(CONN, "Stop session handler %s/%d\n", task->comm, task_pid_nr(task)); - conn->status = KSMBD_SESS_EXITING; + ksmbd_conn_set_exiting(conn); if (t->ops->shutdown) { read_unlock(&conn_list_lock); t->ops->shutdown(t); diff --git a/fs/ksmbd/connection.h b/fs/ksmbd/connection.h index fd243cdddb22..62ffc1c05c27 100644 --- a/fs/ksmbd/connection.h +++ b/fs/ksmbd/connection.h @@ -154,6 +154,8 @@ void ksmbd_conn_init_server_callbacks(struct ksmbd_conn_ops *ops); int ksmbd_conn_handler_loop(void *p); int ksmbd_conn_transport_init(void); void ksmbd_conn_transport_destroy(void); +void ksmbd_conn_lock(struct ksmbd_conn *conn); +void ksmbd_conn_unlock(struct ksmbd_conn *conn); /* * WARNING @@ -161,43 +163,48 @@ void ksmbd_conn_transport_destroy(void); * This is a hack. We will move status to a proper place once we land * a multi-sessions support. */ -static inline bool ksmbd_conn_good(struct ksmbd_work *work) +static inline bool ksmbd_conn_good(struct ksmbd_conn *conn) { - return work->conn->status == KSMBD_SESS_GOOD; + return READ_ONCE(conn->status) == KSMBD_SESS_GOOD; } -static inline bool ksmbd_conn_need_negotiate(struct ksmbd_work *work) +static inline bool ksmbd_conn_need_negotiate(struct ksmbd_conn *conn) { - return work->conn->status == KSMBD_SESS_NEED_NEGOTIATE; + return READ_ONCE(conn->status) == KSMBD_SESS_NEED_NEGOTIATE; } -static inline bool ksmbd_conn_need_reconnect(struct ksmbd_work *work) +static inline bool ksmbd_conn_need_reconnect(struct ksmbd_conn *conn) { - return work->conn->status == KSMBD_SESS_NEED_RECONNECT; + return READ_ONCE(conn->status) == KSMBD_SESS_NEED_RECONNECT; } -static inline bool ksmbd_conn_exiting(struct ksmbd_work *work) +static inline bool ksmbd_conn_exiting(struct ksmbd_conn *conn) { - return work->conn->status == KSMBD_SESS_EXITING; + return READ_ONCE(conn->status) == KSMBD_SESS_EXITING; } -static inline void ksmbd_conn_set_good(struct ksmbd_work *work) +static inline void ksmbd_conn_set_new(struct ksmbd_conn *conn) { - work->conn->status = KSMBD_SESS_GOOD; + WRITE_ONCE(conn->status, KSMBD_SESS_NEW); } -static inline void ksmbd_conn_set_need_negotiate(struct ksmbd_work *work) +static inline void ksmbd_conn_set_good(struct ksmbd_conn *conn) { - work->conn->status = KSMBD_SESS_NEED_NEGOTIATE; + WRITE_ONCE(conn->status, KSMBD_SESS_GOOD); } -static inline void ksmbd_conn_set_need_reconnect(struct ksmbd_work *work) +static inline void ksmbd_conn_set_need_negotiate(struct ksmbd_conn *conn) { - work->conn->status = KSMBD_SESS_NEED_RECONNECT; + WRITE_ONCE(conn->status, KSMBD_SESS_NEED_NEGOTIATE); } -static inline void ksmbd_conn_set_exiting(struct ksmbd_work *work) +static inline void ksmbd_conn_set_need_reconnect(struct ksmbd_conn *conn) { - work->conn->status = KSMBD_SESS_EXITING; + WRITE_ONCE(conn->status, KSMBD_SESS_NEED_RECONNECT); +} + +static inline void ksmbd_conn_set_exiting(struct ksmbd_conn *conn) +{ + WRITE_ONCE(conn->status, KSMBD_SESS_EXITING); } #endif /* __CONNECTION_H__ */ diff --git a/fs/ksmbd/mgmt/user_session.c b/fs/ksmbd/mgmt/user_session.c index a2b128dedcfc..69b85a98e2c3 100644 --- a/fs/ksmbd/mgmt/user_session.c +++ b/fs/ksmbd/mgmt/user_session.c @@ -324,6 +324,7 @@ static struct ksmbd_session *__session_create(int protocol) if (ksmbd_init_file_table(&sess->file_table)) goto error; + sess->state = SMB2_SESSION_IN_PROGRESS; set_session_flag(sess, protocol); xa_init(&sess->tree_conns); xa_init(&sess->ksmbd_chann_list); diff --git a/fs/ksmbd/server.c b/fs/ksmbd/server.c index eb45d56b3577..656ef21db1fb 100644 --- a/fs/ksmbd/server.c +++ b/fs/ksmbd/server.c @@ -93,7 +93,8 @@ static inline int check_conn_state(struct ksmbd_work *work) { struct smb_hdr *rsp_hdr; - if (ksmbd_conn_exiting(work) || ksmbd_conn_need_reconnect(work)) { + if (ksmbd_conn_exiting(work->conn) || + ksmbd_conn_need_reconnect(work->conn)) { rsp_hdr = work->response_buf; rsp_hdr->Status.CifsError = STATUS_CONNECTION_DISCONNECTED; return 1; diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c index 0cb263532cc4..ce29e54976e2 100644 --- a/fs/ksmbd/smb2pdu.c +++ b/fs/ksmbd/smb2pdu.c @@ -269,7 +269,7 @@ int init_smb2_neg_rsp(struct ksmbd_work *work) rsp = work->response_buf; - WARN_ON(ksmbd_conn_good(work)); + WARN_ON(ksmbd_conn_good(conn)); rsp->StructureSize = cpu_to_le16(65); ksmbd_debug(SMB, "conn->dialect 0x%x\n", conn->dialect); @@ -299,7 +299,7 @@ int init_smb2_neg_rsp(struct ksmbd_work *work) rsp->SecurityMode |= SMB2_NEGOTIATE_SIGNING_REQUIRED_LE; conn->use_spnego = true; - ksmbd_conn_set_need_negotiate(work); + ksmbd_conn_set_need_negotiate(conn); return 0; } @@ -588,8 +588,8 @@ int smb2_check_user_session(struct ksmbd_work *work) cmd == SMB2_SESSION_SETUP_HE) return 0; - if (!ksmbd_conn_good(work)) - return -EIO; + if (!ksmbd_conn_good(conn)) + return -EINVAL; sess_id = le64_to_cpu(req_hdr->SessionId); @@ -639,7 +639,7 @@ static void destroy_previous_session(struct ksmbd_conn *conn, prev_sess->state = SMB2_SESSION_EXPIRED; xa_for_each(&prev_sess->ksmbd_chann_list, index, chann) - chann->conn->status = KSMBD_SESS_EXITING; + ksmbd_conn_set_exiting(chann->conn); } /** @@ -1103,7 +1103,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) ksmbd_debug(SMB, "Received negotiate request\n"); conn->need_neg = false; - if (ksmbd_conn_good(work)) { + if (ksmbd_conn_good(conn)) { pr_err("conn->tcp_status is already in CifsGood State\n"); work->send_no_response = 1; return rc; @@ -1258,7 +1258,7 @@ int smb2_handle_negotiate(struct ksmbd_work *work) } conn->srv_sec_mode = le16_to_cpu(rsp->SecurityMode); - ksmbd_conn_set_need_negotiate(work); + ksmbd_conn_set_need_negotiate(conn); err_out: if (rc < 0) @@ -1679,6 +1679,7 @@ int smb2_sess_setup(struct ksmbd_work *work) rsp->SecurityBufferLength = 0; inc_rfc1001_len(rsp, 9); + ksmbd_conn_lock(conn); if (!req->hdr.SessionId) { sess = ksmbd_smb2_session_create(); if (!sess) { @@ -1726,6 +1727,12 @@ int smb2_sess_setup(struct ksmbd_work *work) goto out_err; } + if (ksmbd_conn_need_reconnect(conn)) { + rc = -EFAULT; + sess = NULL; + goto out_err; + } + if (ksmbd_session_lookup(conn, sess_id)) { rc = -EACCES; goto out_err; @@ -1750,12 +1757,20 @@ int smb2_sess_setup(struct ksmbd_work *work) rc = -ENOENT; goto out_err; } + + if (sess->state == SMB2_SESSION_EXPIRED) { + rc = -EFAULT; + goto out_err; + } + + if (ksmbd_conn_need_reconnect(conn)) { + rc = -EFAULT; + sess = NULL; + goto out_err; + } } work->sess = sess; - if (sess->state == SMB2_SESSION_EXPIRED) - sess->state = SMB2_SESSION_IN_PROGRESS; - negblob_off = le16_to_cpu(req->SecurityBufferOffset); negblob_len = le16_to_cpu(req->SecurityBufferLength); if (negblob_off < (offsetof(struct smb2_sess_setup_req, Buffer) - 4) || @@ -1785,8 +1800,10 @@ int smb2_sess_setup(struct ksmbd_work *work) goto out_err; } - ksmbd_conn_set_good(work); - sess->state = SMB2_SESSION_VALID; + if (!ksmbd_conn_need_reconnect(conn)) { + ksmbd_conn_set_good(conn); + sess->state = SMB2_SESSION_VALID; + } kfree(sess->Preauth_HashValue); sess->Preauth_HashValue = NULL; } else if (conn->preferred_auth_mech == KSMBD_AUTH_NTLMSSP) { @@ -1807,8 +1824,10 @@ int smb2_sess_setup(struct ksmbd_work *work) if (rc) goto out_err; - ksmbd_conn_set_good(work); - sess->state = SMB2_SESSION_VALID; + if (!ksmbd_conn_need_reconnect(conn)) { + ksmbd_conn_set_good(conn); + sess->state = SMB2_SESSION_VALID; + } if (conn->binding) { struct preauth_session *preauth_sess; @@ -1876,14 +1895,13 @@ int smb2_sess_setup(struct ksmbd_work *work) if (sess->user && sess->user->flags & KSMBD_USER_FLAG_DELAY_SESSION) try_delay = true; - xa_erase(&conn->sessions, sess->id); - ksmbd_session_destroy(sess); - work->sess = NULL; + sess->state = SMB2_SESSION_EXPIRED; if (try_delay) ssleep(5); } } + ksmbd_conn_unlock(conn); return rc; } @@ -2102,21 +2120,24 @@ int smb2_session_logoff(struct ksmbd_work *work) { struct ksmbd_conn *conn = work->conn; struct smb2_logoff_rsp *rsp = work->response_buf; - struct ksmbd_session *sess = work->sess; + struct ksmbd_session *sess; + struct smb2_logoff_req *req = work->request_buf; rsp->StructureSize = cpu_to_le16(4); inc_rfc1001_len(rsp, 4); ksmbd_debug(SMB, "request\n"); - /* setting CifsExiting here may race with start_tcp_sess */ - ksmbd_conn_set_need_reconnect(work); + ksmbd_conn_set_need_reconnect(conn); ksmbd_close_session_fds(work); ksmbd_conn_wait_idle(conn); + /* + * Re-lookup session to validate if session is deleted + * while waiting request complete + */ + sess = ksmbd_session_lookup(conn, le64_to_cpu(req->hdr.SessionId)); if (ksmbd_tree_conn_session_logoff(sess)) { - struct smb2_logoff_req *req = work->request_buf; - ksmbd_debug(SMB, "Invalid tid %d\n", req->hdr.Id.SyncId.TreeId); rsp->hdr.Status = STATUS_NETWORK_NAME_DELETED; smb2_set_err_rsp(work); @@ -2128,9 +2149,7 @@ int smb2_session_logoff(struct ksmbd_work *work) ksmbd_free_user(sess->user); sess->user = NULL; - - /* let start_tcp_sess free connection info now */ - ksmbd_conn_set_need_negotiate(work); + ksmbd_conn_set_need_negotiate(conn); return 0; } diff --git a/fs/ksmbd/transport_tcp.c b/fs/ksmbd/transport_tcp.c index d1d7954368a5..42504e80f2af 100644 --- a/fs/ksmbd/transport_tcp.c +++ b/fs/ksmbd/transport_tcp.c @@ -333,7 +333,7 @@ static int ksmbd_tcp_readv(struct tcp_transport *t, struct kvec *iov_orig, if (length == -EINTR) { total_read = -ESHUTDOWN; break; - } else if (conn->status == KSMBD_SESS_NEED_RECONNECT) { + } else if (ksmbd_conn_need_reconnect(conn)) { total_read = -EAGAIN; break; } else if (length == -ERESTARTSYS || length == -EAGAIN) {