Attachment 17483 Details for Bug 15128 – patch for v4.15

[patch] patch for v4.15

15128-v4.15-test.patch (text/plain), 13.67 KB, created by Noel Power on 2022-08-18 16:07:17 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Noel Power

Created: 2022-08-18 16:07:17 UTC

Size: 13.67 KB

patch

obsolete

>From f46042eea00755ac5e55b305d78f1ec865993b82 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Tue, 16 Aug 2022 13:51:27 -0700
>Subject: [PATCH 1/5] s3/smbd: Use after free when iterating
> smbd_server_connection->connections
>
>In SMB2 smbd_smb2_tree_connect() we create a new conn struct
>inside make_connection_smb2() then move the ownership to tcon using:
>
>        tcon->compat = talloc_move(tcon, &compat_conn);
>
>so the lifetime of tcon->compat is tied directly to tcon.
>
>Inside smbXsrv_tcon_disconnect() we have:
>
> 908                 ok = chdir_current_service(tcon->compat);
> 909                 if (!ok) {
> 910                         status = NT_STATUS_INTERNAL_ERROR;
> 911                         DEBUG(0, ("smbXsrv_tcon_disconnect(0x%08x, '%s'): "
> 912                                   "chdir_current_service() failed: %s\n",
> 913                                   tcon->global->tcon_global_id,
> 914                                   tcon->global->share_name,
> 915                                   nt_errstr(status)));
> 916                         tcon->compat = NULL;
> 917                         return status;
> 918                 }
> 919
> 920                 close_cnum(tcon->compat, vuid);
> 921                 tcon->compat = NULL;
>
>If chdir_current_service(tcon->compat) fails, we return status without ever having
>called close_cnum(tcon->compat, vuid), leaving the conn pointer left in the linked
>list sconn->connections.
>
>The caller frees tcon and (by ownership) tcon->compat, still leaving the
>freed tcon->compat pointer on the sconn->connections linked list.
>
>When deadtime_fn() fires and walks the sconn->connections list it
>indirects this freed pointer. We must call close_cnum() on error also.
>
>Valgrind trace from Noel Power <noel.power@suse.com> is:
>
>==6432== Invalid read of size 8
>==6432==    at 0x52CED3A: conn_lastused_update (conn_idle.c:38)
>==6432==    by 0x52CEDB1: conn_idle_all (conn_idle.c:54)
>==6432==    by 0x5329971: deadtime_fn (smb2_process.c:1566)
>==6432==    by 0x5DA2339: smbd_idle_event_handler (util_event.c:45)
>==6432==    by 0x685F2F8: tevent_common_invoke_timer_handler (tevent_timed.c:376)
>
>==6432==  Address 0x19074b88 is 232 bytes inside a block of size 328 free'd
>==6432==    at 0x4C3451B: free (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>==6432==    by 0x5B38521: _tc_free_internal (talloc.c:1222)
>==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
>==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
>==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
>==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
>==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
>==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
>==6432==    by 0x5B39463: _tc_free_children_internal (talloc.c:1669)
>==6432==    by 0x5B38404: _tc_free_internal (talloc.c:1184)
>==6432==    by 0x5B385C5: _talloc_free_internal (talloc.c:1248)
>==6432==    by 0x5B3988D: _talloc_free (talloc.c:1792)
>==6432==    by 0x5349B22: smbd_smb2_flush_send_queue (smb2_server.c:4828)
>
>==6432==  Block was alloc'd at
>==6432==    at 0x4C332EF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
>==6432==    by 0x5B378D9: __talloc_with_prefix (talloc.c:783)
>==6432==    by 0x5B37A73: __talloc (talloc.c:825)
>==6432==    by 0x5B37E0C: _talloc_named_const (talloc.c:982)
>==6432==    by 0x5B3A8ED: _talloc_zero (talloc.c:2421)
>==6432==    by 0x539873A: conn_new (conn.c:70)
>==6432==    by 0x532D692: make_connection_smb2 (smb2_service.c:909)
>==6432==    by 0x5352B5E: smbd_smb2_tree_connect (smb2_tcon.c:344)
>
>https://bugzilla.samba.org/show_bug.cgi?id=15128
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>(cherry picked from commit 0bdfb5a5e60df214c088df0782c4a1bcc2a4944a)
>---
> source3/smbd/smbXsrv_tcon.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
>diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c
>index 6b105522855..b515b19e88f 100644
>--- a/source3/smbd/smbXsrv_tcon.c
>+++ b/source3/smbd/smbXsrv_tcon.c
>@@ -913,6 +913,15 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
> 				  tcon->global->tcon_global_id,
> 				  tcon->global->share_name,
> 				  nt_errstr(status)));
>+			/*
>+			 * We must call close_cnum() on
>+			 * error, as the caller is going
>+			 * to free tcon and tcon->compat
>+			 * so we must ensure tcon->compat is
>+			 * removed from the linked list
>+			 * conn->sconn->connections.
>+			 */
>+			close_cnum(tcon->compat, vuid);
> 			tcon->compat = NULL;
> 			return status;
> 		}
>-- 
>2.35.3
>
>
>From dedfc3766d516da0fde1e1d2820ef51134e3588e Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Fri, 22 Jul 2022 16:28:03 +0100
>Subject: [PATCH 2/5] s3/smbd: Use after free when iterating
> smbd_server_connection->connections
>
>Change conn_free() to just use a destructor. We now
>catch any other places where we may have forgetten to
>call conn_free() - it's implicit on talloc_free(conn).
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128
>
>Based on code from Noel Power <noel.power@suse.com>.
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>
>Autobuild-User(master): Noel Power <npower@samba.org>
>Autobuild-Date(master): Wed Aug 17 09:54:06 UTC 2022 on sn-devel-184
>
>(cherry picked from commit f92bacbe216d2d74ea3ccf3fe0df5c1cc9860996)
>---
> source3/smbd/conn.c | 36 +++++++++++++++++++++++++-----------
> 1 file changed, 25 insertions(+), 11 deletions(-)
>
>diff --git a/source3/smbd/conn.c b/source3/smbd/conn.c
>index 044242d5697..776d7af4c12 100644
>--- a/source3/smbd/conn.c
>+++ b/source3/smbd/conn.c
>@@ -24,6 +24,25 @@
> #include "smbd/globals.h"
> #include "lib/util/bitmap.h"
> 
>+static void conn_free_internal(connection_struct *conn);
>+
>+/****************************************************************************
>+ * Remove a conn struct from conn->sconn->connections
>+ * if not already done.
>+****************************************************************************/
>+
>+static int conn_struct_destructor(connection_struct *conn)
>+{
>+        if (conn->sconn != NULL) {
>+		DLIST_REMOVE(conn->sconn->connections, conn);
>+		SMB_ASSERT(conn->sconn->num_connections > 0);
>+		conn->sconn->num_connections--;
>+		conn->sconn = NULL;
>+	}
>+	conn_free_internal(conn);
>+	return 0;
>+}
>+
> /****************************************************************************
>  Return the number of open connections.
> ****************************************************************************/
>@@ -115,6 +134,11 @@ connection_struct *conn_new(struct smbd_server_connection *sconn)
> 	DLIST_ADD(sconn->connections, conn);
> 	sconn->num_connections++;
> 
>+	/*
>+	 * Catches the case where someone forgets to call
>+	 * conn_free().
>+	 */
>+	talloc_set_destructor(conn, conn_struct_destructor);
> 	return conn;
> }
> 
>@@ -212,7 +236,6 @@ static void conn_free_internal(connection_struct *conn)
> 	free_namearray(conn->aio_write_behind_list);
> 
> 	ZERO_STRUCTP(conn);
>-	talloc_destroy(conn);
> }
> 
> /****************************************************************************
>@@ -221,16 +244,7 @@ static void conn_free_internal(connection_struct *conn)
> 
> void conn_free(connection_struct *conn)
> {
>-	if (conn->sconn == NULL) {
>-		conn_free_internal(conn);
>-		return;
>-	}
>-
>-	DLIST_REMOVE(conn->sconn->connections, conn);
>-	SMB_ASSERT(conn->sconn->num_connections > 0);
>-	conn->sconn->num_connections--;
>-
>-	conn_free_internal(conn);
>+	TALLOC_FREE(conn);
> }
> 
> /*
>-- 
>2.35.3
>
>
>From b03a1638f5da9f8e1344a520ebe4312f7cc2c4a3 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 17 Aug 2022 11:35:29 -0700
>Subject: [PATCH 3/5] s3: smbd: Add "enum file_close_type close_type" parameter
> to close_cnum().
>
>Not yet used, but needed so we can differentiate between
>SHUTDOWN_CLOSE and ERROR_CLOSE in smbXsrv_tcon_disconnect()
>if we fail to chdir. In that case we want to close the fd,
>but not run any delete-on-close actions.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>(cherry picked from commit 9203d17106c0e55a30813ff1ed76869c7581a343)
>[npower@samba.org Adjusted for 4.15 filename change
>         smb2-service.c -> service.c]
>---
> source3/smbd/proto.h        | 4 +++-
> source3/smbd/service.c      | 4 +++-
> source3/smbd/smbXsrv_tcon.c | 4 ++--
> 3 files changed, 8 insertions(+), 4 deletions(-)
>
>diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
>index 46d3f2e5156..33d53962e66 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -1127,7 +1127,9 @@ connection_struct *make_connection(struct smb_request *req,
> 				   const char *service_in,
> 				   const char *pdev, uint64_t vuid,
> 				   NTSTATUS *status);
>-void close_cnum(connection_struct *conn, uint64_t vuid);
>+void close_cnum(connection_struct *conn,
>+		uint64_t vuid,
>+		enum file_close_type close_type);
> 
> /* The following definitions come from smbd/session.c  */
> struct sessionid;
>diff --git a/source3/smbd/service.c b/source3/smbd/service.c
>index ef7c14d92d0..b07239cc6fe 100644
>--- a/source3/smbd/service.c
>+++ b/source3/smbd/service.c
>@@ -1111,7 +1111,9 @@ connection_struct *make_connection(struct smb_request *req,
>  Close a cnum.
> ****************************************************************************/
> 
>-void close_cnum(connection_struct *conn, uint64_t vuid)
>+void close_cnum(connection_struct *conn,
>+		uint64_t vuid,
>+		enum file_close_type close_type)
> {
> 	char rootpath[2] = { '/', '\0'};
> 	struct smb_filename root_fname = { .base_name = rootpath };
>diff --git a/source3/smbd/smbXsrv_tcon.c b/source3/smbd/smbXsrv_tcon.c
>index b515b19e88f..8707082edd6 100644
>--- a/source3/smbd/smbXsrv_tcon.c
>+++ b/source3/smbd/smbXsrv_tcon.c
>@@ -921,12 +921,12 @@ NTSTATUS smbXsrv_tcon_disconnect(struct smbXsrv_tcon *tcon, uint64_t vuid)
> 			 * removed from the linked list
> 			 * conn->sconn->connections.
> 			 */
>-			close_cnum(tcon->compat, vuid);
>+			close_cnum(tcon->compat, vuid, ERROR_CLOSE);
> 			tcon->compat = NULL;
> 			return status;
> 		}
> 
>-		close_cnum(tcon->compat, vuid);
>+		close_cnum(tcon->compat, vuid, SHUTDOWN_CLOSE);
> 		tcon->compat = NULL;
> 	}
> 
>-- 
>2.35.3
>
>
>From fb7267409e5f41bfe160d2e4ce1a4ba61d7a0780 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 17 Aug 2022 11:39:36 -0700
>Subject: [PATCH 4/5] s3: smbd: Add "enum file_close_type close_type" parameter
> to file_close_conn().
>
>Not yet used.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reviewed-by: Noel Power <npower@samba.org>
>(cherry picked from commit 7005a6354df5522d9f665fb30052c458dfc93124)
>[npower@samba.org Adjusted for 4.15 filename change
>         smb2-service.c -> service.c]
>---
> source3/smbd/files.c   | 2 +-
> source3/smbd/proto.h   | 2 +-
> source3/smbd/service.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
>diff --git a/source3/smbd/files.c b/source3/smbd/files.c
>index 196c0008e31..582ca0566cc 100644
>--- a/source3/smbd/files.c
>+++ b/source3/smbd/files.c
>@@ -755,7 +755,7 @@ NTSTATUS parent_pathref(TALLOC_CTX *mem_ctx,
>  Close all open files for a connection.
> ****************************************************************************/
> 
>-void file_close_conn(connection_struct *conn)
>+void file_close_conn(connection_struct *conn, enum file_close_type close_type)
> {
> 	files_struct *fsp, *next;
> 
>diff --git a/source3/smbd/proto.h b/source3/smbd/proto.h
>index 33d53962e66..d937d3d9615 100644
>--- a/source3/smbd/proto.h
>+++ b/source3/smbd/proto.h
>@@ -397,7 +397,7 @@ void fsp_set_gen_id(files_struct *fsp);
> NTSTATUS file_new(struct smb_request *req, connection_struct *conn,
> 		  files_struct **result);
> NTSTATUS fsp_bind_smb(struct files_struct *fsp, struct smb_request *req);
>-void file_close_conn(connection_struct *conn);
>+void file_close_conn(connection_struct *conn, enum file_close_type close_type);
> bool file_init_global(void);
> bool file_init(struct smbd_server_connection *sconn);
> void file_close_user(struct smbd_server_connection *sconn, uint64_t vuid);
>diff --git a/source3/smbd/service.c b/source3/smbd/service.c
>index b07239cc6fe..d4dc81bf986 100644
>--- a/source3/smbd/service.c
>+++ b/source3/smbd/service.c
>@@ -1120,7 +1120,7 @@ void close_cnum(connection_struct *conn,
> 	const struct loadparm_substitution *lp_sub =
> 		loadparm_s3_global_substitution();
> 
>-	file_close_conn(conn);
>+	file_close_conn(conn, close_type);
> 
> 	change_to_root_user();
> 
>-- 
>2.35.3
>
>
>From fe65b252c7462e8f9c380586d7f613e3e778d4f3 Mon Sep 17 00:00:00 2001
>From: Jeremy Allison <jra@samba.org>
>Date: Wed, 17 Aug 2022 11:43:47 -0700
>Subject: [PATCH 5/5] s3: smbd: Plumb close_type parameter through
> close_file_in_loop(), file_close_conn()
>
>Allows close_file_in_loop() to differentiate between SHUTDOWN_CLOSE
>(previously it only used this close type) and ERROR_CLOSE - called
>on error from smbXsrv_tcon_disconnect() in the error path. In that
>case we want to close the fd, but not run any delete-on-close actions.
>
>BUG: https://bugzilla.samba.org/show_bug.cgi?id=15128
>
>Signed-off-by: Jeremy Allison <jra@samba.org>
>Reivewed-by: Noel Power <npower@samba.org>
>
>Autobuild-User(master): Noel Power <npower@samba.org>
>Autobuild-Date(master): Thu Aug 18 14:10:18 UTC 2022 on sn-devel-184
>
>(cherry picked from commit cf5f7b1489930f6d64c3e3512f116ccf286d4605)
>[npower@samba.org Adjusted for 4.15 only file_close_conn needs to
>     differentiate between SHUTDOWN_CLOSE & ERROR_CLOSE]
>---
> source3/smbd/files.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/source3/smbd/files.c b/source3/smbd/files.c
>index 582ca0566cc..3f90c2e247e 100644
>--- a/source3/smbd/files.c
>+++ b/source3/smbd/files.c
>@@ -770,7 +770,7 @@ void file_close_conn(connection_struct *conn, enum file_close_type close_type)
> 			 */
> 			fsp->op->global->durable = false;
> 		}
>-		close_file(NULL, fsp, SHUTDOWN_CLOSE);
>+		close_file(NULL, fsp, close_type);
> 	}
> }
> 
>-- 
>2.35.3
>

Flags:

jra: review+

Actions: View

Attachments on bug 15128: 17440 | 17441 | 17442 | 17443 | 17444 | 17457 | 17458 | 17459 | 17478 | 17479 | 17481 | 17482 | 17483