The Samba-Bugzilla – Attachment 18112 Details for
Bug 15423
use-after-free in aio_del_req_from_fsp during smbd shutdown after failed IPC FSCTL_PIPE_TRANSCEIVE
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
git-am fix for 4.19.next, 4.18.next.
bug-15423-4.19.patch (text/plain), 14.77 KB, created by
Jeremy Allison
on 2023-09-20 17:09:36 UTC
(
hide
)
Description:
git-am fix for 4.19.next, 4.18.next.
Filename:
MIME Type:
Creator:
Jeremy Allison
Created:
2023-09-20 17:09:36 UTC
Size:
14.77 KB
patch
obsolete
>From a88f0e7f38670966bf1553fa5036d650b74c666a Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 18 Sep 2023 14:43:23 -0700 >Subject: [PATCH 1/5] s3: smbd: Add some DEVELOPER-only code to panic if the > destructor for an aio_lnk is called and the associated fsp doesn't exist. > >Make this DEVELOPER-only as it walks the entire open >file list on every file close (with associated aio). > >This helps catch really subtle problems with orphaned >aio lnk structs. > >Reproducer test case to follow. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 82e88f70f181300f6f98691f6680839a94470e13) >--- > source3/smbd/smb2_aio.c | 24 ++++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > >diff --git a/source3/smbd/smb2_aio.c b/source3/smbd/smb2_aio.c >index 8c01c76a3e9..88aa68d218f 100644 >--- a/source3/smbd/smb2_aio.c >+++ b/source3/smbd/smb2_aio.c >@@ -64,6 +64,9 @@ struct aio_extra *create_aio_extra(TALLOC_CTX *mem_ctx, > } > > struct aio_req_fsp_link { >+#ifdef DEVELOPER >+ struct smbd_server_connection *sconn; >+#endif > files_struct *fsp; > struct tevent_req *req; > }; >@@ -74,6 +77,24 @@ static int aio_del_req_from_fsp(struct aio_req_fsp_link *lnk) > files_struct *fsp = lnk->fsp; > struct tevent_req *req = lnk->req; > >+#ifdef DEVELOPER >+ struct files_struct *ifsp = NULL; >+ bool found = false; >+ >+ /* >+ * When this is called, lnk->fsp must still exist >+ * on the files list for this connection. Panic if not. >+ */ >+ for (ifsp = lnk->sconn->files; ifsp; ifsp = ifsp->next) { >+ if (ifsp == fsp) { >+ found = true; >+ } >+ } >+ if (!found) { >+ smb_panic("orphaned lnk on fsp aio list.\n"); >+ } >+#endif >+ > for (i=0; i<fsp->num_aio_requests; i++) { > if (fsp->aio_requests[i] == req) { > break; >@@ -130,6 +151,9 @@ bool aio_add_req_to_fsp(files_struct *fsp, struct tevent_req *req) > > lnk->fsp = fsp; > lnk->req = req; >+#ifdef DEVELOPER >+ lnk->sconn = fsp->conn->sconn; >+#endif > talloc_set_destructor(lnk, aio_del_req_from_fsp); > > return true; >-- >2.39.3 > > >From f6e8e599a58d17dede3fc09a20258b0cd263df76 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 18 Sep 2023 17:09:00 -0700 >Subject: [PATCH 2/5] s3: smbd: named pipe reads are async. Use the same logic > as for named pipe transacts to avoid crashes on shutdown. > >Noticed by Metze. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 3f32bf887d4425655e81da0b2234cbca3b1d56e6) >--- > source3/smbd/smb2_read.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > >diff --git a/source3/smbd/smb2_read.c b/source3/smbd/smb2_read.c >index 0101c42cf76..bac78e4c77a 100644 >--- a/source3/smbd/smb2_read.c >+++ b/source3/smbd/smb2_read.c >@@ -494,6 +494,7 @@ static struct tevent_req *smbd_smb2_read_send(TALLOC_CTX *mem_ctx, > > if (IS_IPC(smbreq->conn)) { > struct tevent_req *subreq = NULL; >+ bool ok; > > state->out_data = data_blob_talloc(state, NULL, in_length); > if (in_length > 0 && tevent_req_nomem(state->out_data.data, req)) { >@@ -515,6 +516,18 @@ static struct tevent_req *smbd_smb2_read_send(TALLOC_CTX *mem_ctx, > tevent_req_set_callback(subreq, > smbd_smb2_read_pipe_done, > req); >+ >+ /* >+ * Make sure we mark the fsp as having outstanding async >+ * activity so we don't crash on shutdown close. >+ */ >+ >+ ok = aio_add_req_to_fsp(fsp, req); >+ if (!ok) { >+ tevent_req_nterror(req, NT_STATUS_NO_MEMORY); >+ return tevent_req_post(req, ev); >+ } >+ > return req; > } > >-- >2.39.3 > > >From b1c55e1432ad42af21e756787b3ec6a235ff7f47 Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Mon, 18 Sep 2023 17:37:44 -0700 >Subject: [PATCH 3/5] s3: smbd: named pipe writes are async. Use the same logic > as for named pipe transacts to avoid crashes on shutdown. > >Noticed by Metze. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit ea062c3b0d4dbb1f0682f808ac893bf36a6fb194) >--- > source3/smbd/smb2_write.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > >diff --git a/source3/smbd/smb2_write.c b/source3/smbd/smb2_write.c >index 269c489642e..a7af20173a1 100644 >--- a/source3/smbd/smb2_write.c >+++ b/source3/smbd/smb2_write.c >@@ -307,6 +307,7 @@ static struct tevent_req *smbd_smb2_write_send(TALLOC_CTX *mem_ctx, > > if (IS_IPC(smbreq->conn)) { > struct tevent_req *subreq = NULL; >+ bool ok; > > if (!fsp_is_np(fsp)) { > tevent_req_nterror(req, NT_STATUS_FILE_CLOSED); >@@ -323,6 +324,18 @@ static struct tevent_req *smbd_smb2_write_send(TALLOC_CTX *mem_ctx, > tevent_req_set_callback(subreq, > smbd_smb2_write_pipe_done, > req); >+ >+ /* >+ * Make sure we mark the fsp as having outstanding async >+ * activity so we don't crash on shutdown close. >+ */ >+ >+ ok = aio_add_req_to_fsp(fsp, req); >+ if (!ok) { >+ tevent_req_nterror(req, NT_STATUS_NO_MEMORY); >+ return tevent_req_post(req, ev); >+ } >+ > return req; > } > >-- >2.39.3 > > >From 5365769e354a90a4d17cfef96026094974c1296b Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 19 Sep 2023 14:30:26 -0700 >Subject: [PATCH 4/5] s3: torture: Add a new SMB2 test: > SMB2-PIPE-READ-ASYNC-DISCONNECT > >Shows the server crashes if we open a named pipe, do an async read >and then disconnect. > >Adds knownfail: > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> >(cherry picked from commit 66398dd03c46633b474438dddb771caa2d245e64) >--- > .../smb2_pipe_read_async_disconnect | 1 + > .../tests/test_smbtorture_nocrash_s3.sh | 38 ++++++ > source3/selftest/tests.py | 16 +++ > source3/torture/proto.h | 1 + > source3/torture/test_smb2.c | 117 ++++++++++++++++++ > source3/torture/torture.c | 4 + > 6 files changed, 177 insertions(+) > create mode 100644 selftest/knownfail.d/smb2_pipe_read_async_disconnect > create mode 100755 source3/script/tests/test_smbtorture_nocrash_s3.sh > >diff --git a/selftest/knownfail.d/smb2_pipe_read_async_disconnect b/selftest/knownfail.d/smb2_pipe_read_async_disconnect >new file mode 100644 >index 00000000000..342a308adec >--- /dev/null >+++ b/selftest/knownfail.d/smb2_pipe_read_async_disconnect >@@ -0,0 +1 @@ >+^samba3.smbtorture_s3.plain.SMB2-PIPE-READ-ASYNC-DISCONNECT.check_panic\(fileserver\) >diff --git a/source3/script/tests/test_smbtorture_nocrash_s3.sh b/source3/script/tests/test_smbtorture_nocrash_s3.sh >new file mode 100755 >index 00000000000..b6ef1391262 >--- /dev/null >+++ b/source3/script/tests/test_smbtorture_nocrash_s3.sh >@@ -0,0 +1,38 @@ >+#!/bin/sh >+ >+. $(dirname $0)/../../../testprogs/blackbox/subunit.sh >+ >+# this runs the file serving tests that are expected to pass with samba3 >+ >+if [ $# -lt 5 ]; then >+ cat <<EOF >+Usage: test_smbtorture_s3.sh TEST UNC USERNAME PASSWORD SMBTORTURE <smbtorture args> >+EOF >+ exit 1 >+fi >+ >+t="$1" >+unc="$2" >+username="$3" >+password="$4" >+SMBTORTURE="$5" >+shift 5 >+ADDARGS="$*" >+ >+incdir=$(dirname $0)/../../../testprogs/blackbox >+. $incdir/subunit.sh >+ >+panic_count_0=$(grep -c PANIC $SMBD_TEST_LOG) >+ >+echo "$panic_count_0" >/tmp/look >+ >+failed=0 >+testit "smbtorture" $VALGRIND $SMBTORTURE $unc -U"$username"%"$password" $ADDARGS $t || failed=$(expr $failed + 1) >+ >+panic_count_1=$(grep -c PANIC $SMBD_TEST_LOG) >+ >+echo "$panic_count_1" >>/tmp/look >+ >+testit "check_panic" test $panic_count_0 -eq $panic_count_1 || failed=$(expr $failed + 1) >+ >+testok $0 $failed >diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py >index 783cb486012..8986a73f03a 100755 >--- a/source3/selftest/tests.py >+++ b/source3/selftest/tests.py >@@ -481,6 +481,22 @@ plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-DEL-ON-CLOSE-NONWRITE-DELE > "", > "-l $LOCAL_PATH"]) > >+# >+# Test doing an async read + disconnect on a pipe doesn't crash the server. >+# BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 >+# >+plantestsuite("samba3.smbtorture_s3.plain.%s" % "SMB2-PIPE-READ-ASYNC-DISCONNECT", >+ "fileserver", >+ [os.path.join(samba3srcdir, >+ "script/tests/test_smbtorture_nocrash_s3.sh"), >+ 'SMB2-PIPE-READ-ASYNC-DISCONNECT', >+ '//$SERVER_IP/tmp', >+ '$USERNAME', >+ '$PASSWORD', >+ smbtorture3, >+ "", >+ "-l $LOCAL_PATH"]) >+ > shares = [ > "vfs_aio_pthread_async_dosmode_default1", > "vfs_aio_pthread_async_dosmode_default2" >diff --git a/source3/torture/proto.h b/source3/torture/proto.h >index 93d0727e936..0206cf15c9a 100644 >--- a/source3/torture/proto.h >+++ b/source3/torture/proto.h >@@ -124,6 +124,7 @@ bool run_smb2_dfs_paths(int dummy); > bool run_smb2_non_dfs_share(int dummy); > bool run_smb2_dfs_share_non_dfs_path(int dummy); > bool run_smb2_dfs_filename_leading_backslash(int dummy); >+bool run_smb2_pipe_read_async_disconnect(int dummy); > bool run_smb1_dfs_paths(int dummy); > bool run_smb1_dfs_search_paths(int dummy); > bool run_smb1_dfs_operations(int dummy); >diff --git a/source3/torture/test_smb2.c b/source3/torture/test_smb2.c >index 3cb6d13789a..672c38e2b55 100644 >--- a/source3/torture/test_smb2.c >+++ b/source3/torture/test_smb2.c >@@ -5136,3 +5136,120 @@ bool run_smb2_dfs_filename_leading_backslash(int dummy) > (void)smb2_dfs_delete(cli, dfs_filename_slash); > return retval; > } >+ >+/* >+ * Ensure a named pipe async read followed by a disconnect >+ * doesn't crash the server (server crash checked for in >+ * containing test script: >+ * source3/script/tests/test_smbtorture_nocrash_s3.sh) >+ * BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 >+ */ >+ >+bool run_smb2_pipe_read_async_disconnect(int dummy) >+{ >+ struct cli_state *cli = NULL; >+ NTSTATUS status; >+ uint64_t fid_persistent = 0; >+ uint64_t fid_volatile = 0; >+ struct tevent_context *ev; >+ struct tevent_req *req; >+ bool retval = false; >+ >+ printf("Starting SMB2-PIPE-READ-ASYNC-DISCONNECT\n"); >+ >+ if (!torture_init_connection(&cli)) { >+ return false; >+ } >+ >+ status = smbXcli_negprot(cli->conn, >+ cli->timeout, >+ PROTOCOL_SMB2_02, >+ PROTOCOL_SMB3_11); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("smbXcli_negprot returned %s\n", nt_errstr(status)); >+ return false; >+ } >+ >+ status = cli_session_setup_creds(cli, torture_creds); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("cli_session_setup returned %s\n", nt_errstr(status)); >+ return false; >+ } >+ >+ status = cli_tree_connect_creds(cli, "IPC$", "IPC", torture_creds); >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("cli_tree_connect to IPC$ returned %s\n", >+ nt_errstr(status)); >+ return false; >+ } >+ >+ /* Open the SAMR pipe. */ >+ status = smb2cli_create(cli->conn, >+ cli->timeout, >+ cli->smb2.session, >+ cli->smb2.tcon, >+ "SAMR", >+ SMB2_OPLOCK_LEVEL_NONE, /* oplock_level, */ >+ SMB2_IMPERSONATION_IMPERSONATION, /* impersonation_level, */ >+ SEC_STD_SYNCHRONIZE| >+ SEC_FILE_READ_DATA| >+ SEC_FILE_WRITE_DATA, /* desired_access, */ >+ FILE_ATTRIBUTE_NORMAL, /* file_attributes, */ >+ FILE_SHARE_READ|FILE_SHARE_WRITE|FILE_SHARE_DELETE, /* share_access, */ >+ FILE_OPEN, /* create_disposition, */ >+ 0, /* create_options, */ >+ NULL, /* smb2_create_blobs *blobs */ >+ &fid_persistent, >+ &fid_volatile, >+ NULL, /* struct smb_create_returns * */ >+ talloc_tos(), /* mem_ctx. */ >+ NULL, /* struct smb2_create_blobs * */ >+ NULL); /* psymlink */ >+ if (!NT_STATUS_IS_OK(status)) { >+ printf("%s:%d smb2cli_create on SAMR returned %s\n", >+ __FILE__, >+ __LINE__, >+ nt_errstr(status)); >+ goto err; >+ } >+ >+ ev = samba_tevent_context_init(talloc_tos()); >+ if (ev == NULL) { >+ goto err; >+ } >+ >+ /* Start an async read. */ >+ req = smb2cli_read_send(talloc_tos(), >+ ev, >+ cli->conn, >+ cli->timeout, >+ cli->smb2.session, >+ cli->smb2.tcon, >+ 16*1024, >+ 0, /* offset */ >+ fid_persistent, >+ fid_volatile, >+ 0, /* minimum_count */ >+ 0); /* remaining_bytes */ >+ if (req == NULL) { >+ goto err; >+ } >+ >+ /* Force disconnect. */ >+ smbXcli_conn_disconnect(cli->conn, NT_STATUS_LOCAL_DISCONNECT); >+ fid_volatile = 0; >+ retval = true; >+ >+ err: >+ >+ if (fid_volatile != 0) { >+ smb2cli_close(cli->conn, >+ cli->timeout, >+ cli->smb2.session, >+ cli->smb2.tcon, >+ 0, /* flags */ >+ fid_persistent, >+ fid_volatile); >+ } >+ return retval; >+} >diff --git a/source3/torture/torture.c b/source3/torture/torture.c >index 2a8b8dec22f..9a1420c9e3a 100644 >--- a/source3/torture/torture.c >+++ b/source3/torture/torture.c >@@ -15763,6 +15763,10 @@ static struct { > .name = "SMB2-DFS-FILENAME-LEADING-BACKSLASH", > .fn = run_smb2_dfs_filename_leading_backslash, > }, >+ { >+ .name = "SMB2-PIPE-READ-ASYNC-DISCONNECT", >+ .fn = run_smb2_pipe_read_async_disconnect, >+ }, > { > .name = "SMB1-TRUNCATED-SESSSETUP", > .fn = run_smb1_truncated_sesssetup, >-- >2.39.3 > > >From 9747b2e85da0c1613d3a98db23676d1df353eeaf Mon Sep 17 00:00:00 2001 >From: Jeremy Allison <jra@samba.org> >Date: Tue, 19 Sep 2023 14:36:45 -0700 >Subject: [PATCH 5/5] s3: smbd: Ensure we remove any pending aio values for > named pipes on forced shutdown. > >Matches file and directory closes. > >Remove knownfail. > >BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 > >Signed-off-by: Jeremy Allison <jra@samba.org> >Reviewed-by: Ralph Boehme <slow@samba.org> > >Autobuild-User(master): Jeremy Allison <jra@samba.org> >Autobuild-Date(master): Wed Sep 20 02:43:18 UTC 2023 on atb-devel-224 > >(cherry picked from commit 11280f1705c0faa1729f5aeaa1b6a1f79ab5a199) >--- > selftest/knownfail.d/smb2_pipe_read_async_disconnect | 1 - > source3/smbd/close.c | 8 ++++++++ > 2 files changed, 8 insertions(+), 1 deletion(-) > delete mode 100644 selftest/knownfail.d/smb2_pipe_read_async_disconnect > >diff --git a/selftest/knownfail.d/smb2_pipe_read_async_disconnect b/selftest/knownfail.d/smb2_pipe_read_async_disconnect >deleted file mode 100644 >index 342a308adec..00000000000 >--- a/selftest/knownfail.d/smb2_pipe_read_async_disconnect >+++ /dev/null >@@ -1 +0,0 @@ >-^samba3.smbtorture_s3.plain.SMB2-PIPE-READ-ASYNC-DISCONNECT.check_panic\(fileserver\) >diff --git a/source3/smbd/close.c b/source3/smbd/close.c >index 2b180e0c718..af5e78daa10 100644 >--- a/source3/smbd/close.c >+++ b/source3/smbd/close.c >@@ -1630,6 +1630,14 @@ NTSTATUS close_file_smb(struct smb_request *req, > SMB_ASSERT(fsp->stream_fsp == NULL); > > if (fsp->fake_file_handle != NULL) { >+ /* >+ * Named pipes are opened as fake files and >+ * can have pending aio requests. Ensure >+ * we clear out all pending aio on force >+ * shutdown of named pipes also. >+ * BUG: https://bugzilla.samba.org/show_bug.cgi?id=15423 >+ */ >+ assert_no_pending_aio(fsp, close_type); > status = close_fake_file(req, fsp); > } else if (fsp->print_file != NULL) { > /* FIXME: return spool errors */ >-- >2.39.3 >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Raw
Flags:
slow
:
review+
Actions:
View
Attachments on
bug 15423
:
17988
| 18112