From 99729216fde8ee7b649b7cf9cb3745e511413218 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Tue, 18 Oct 2022 16:22:33 -0700 Subject: [PATCH 1/4] s4: torture: Add an async SMB2_OP_FLUSH + SMB2_OP_CLOSE test to smb2.compound_async. Shows we fail sending an SMB2_OP_FLUSH + SMB2_OP_CLOSE compound. Internally the flush goes async and we free the req, then we process the close. When the flush completes it tries to access already freed data. Found using the Apple MacOSX client at SNIA SDC 2022. Add knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (back-ported from commit 17a110c1b58196eb8ecf3c76eb97e8508976c544) --- selftest/knownfail.d/compound_async | 1 + source3/selftest/tests.py | 2 + source4/torture/smb2/compound.c | 117 ++++++++++++++++++++++++++++ source4/torture/smb2/smb2.c | 1 + 4 files changed, 121 insertions(+) create mode 100644 selftest/knownfail.d/compound_async diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async new file mode 100644 index 00000000000..c18465b7204 --- /dev/null +++ b/selftest/knownfail.d/compound_async @@ -0,0 +1 @@ +^samba3.smb2.compound_async.flush_close\(fileserver\) diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py index 72fa5efee3f..e5b0e74c61f 100755 --- a/source3/selftest/tests.py +++ b/source3/selftest/tests.py @@ -1001,6 +1001,8 @@ for t in tests: plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/aio -U$USERNAME%$PASSWORD', 'aio') plansmbtorture4testsuite(t, "ad_dc", '//$SERVER/tmp -U$USERNAME%$PASSWORD') + elif t == "smb2.compound_async": + plansmbtorture4testsuite(t, "fileserver", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD') elif t == "rpc.samba3.netlogon" or t == "rpc.samba3.sessionkey": plansmbtorture4testsuite(t, "nt4_dc_smb1", '//$SERVER_IP/tmp -U$USERNAME%$PASSWORD --option=torture:wksname=samba3rpctest') plansmbtorture4testsuite(t, "ad_dc_smb1", '//$SERVER/tmp -U$USERNAME%$PASSWORD --option=torture:wksname=samba3rpctest') diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c index cf19361130f..e78d78e3a98 100644 --- a/source4/torture/smb2/compound.c +++ b/source4/torture/smb2/compound.c @@ -2057,6 +2057,110 @@ done: return ret; } +static bool test_compound_async_flush_close(struct torture_context *tctx, + struct smb2_tree *tree) +{ + struct smb2_handle fhandle = { .data = { 0, 0 } }; + struct smb2_handle relhandle = { .data = { UINT64_MAX, UINT64_MAX } }; + struct smb2_close cl; + struct smb2_flush fl; + const char *fname = "compound_async_flush_close"; + struct smb2_request *req[2]; + NTSTATUS status; + bool ret = false; + + /* Start clean. */ + smb2_util_unlink(tree, fname); + + /* Create a file. */ + status = torture_smb2_testfile_access(tree, + fname, + &fhandle, + SEC_RIGHTS_FILE_ALL); + CHECK_STATUS(status, NT_STATUS_OK); + + /* Now do a compound flush + close handle. */ + smb2_transport_compound_start(tree->session->transport, 2); + + ZERO_STRUCT(fl); + fl.in.file.handle = fhandle; + + req[0] = smb2_flush_send(tree, &fl); + torture_assert_not_null_goto(tctx, req[0], ret, done, + "smb2_flush_send failed\n"); + + smb2_transport_compound_set_related(tree->session->transport, true); + + ZERO_STRUCT(cl); + cl.in.file.handle = relhandle; + req[1] = smb2_close_send(tree, &cl); + torture_assert_not_null_goto(tctx, req[1], ret, done, + "smb2_close_send failed\n"); + + status = smb2_flush_recv(req[0], &fl); + /* + * On Windows, this flush will usually + * succeed as we have nothing to flush, + * so allow NT_STATUS_OK. Once bug #15172 + * is fixed Samba will do the flush synchronously + * so allow NT_STATUS_OK. + */ + if (!NT_STATUS_IS_OK(status)) { + /* + * If we didn't get NT_STATUS_OK, we *must* + * get NT_STATUS_INTERNAL_ERROR if the flush + * goes async. + * + * For pre-bugfix #15172 Samba, the flush goes async and + * we should get NT_STATUS_INTERNAL_ERROR. + */ + torture_assert_ntstatus_equal_goto(tctx, + status, + NT_STATUS_INTERNAL_ERROR, + ret, + done, + "smb2_flush_recv didn't return " + "NT_STATUS_INTERNAL_ERROR.\n"); + } + status = smb2_close_recv(req[1], &cl); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_close_recv failed."); + + ZERO_STRUCT(fhandle); + + /* + * Do several more operations on the tree, spaced + * out by 1 sec sleeps to make sure the server didn't + * crash on the close. The sleeps are required to + * make test test for a crash reliable, as we ensure + * the pthread fsync internally finishes and accesses + * freed memory. Without them the test occassionally + * passes as we disconnect before the pthread fsync + * finishes. + */ + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OK); + + sleep(1); + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); + + sleep(1); + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); + + ret = true; + + done: + + if (fhandle.data[0] != 0) { + smb2_util_close(tree, fhandle); + } + + smb2_util_unlink(tree, fname); + return ret; +} + struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx) { struct torture_suite *suite = torture_suite_create(ctx, "compound"); @@ -2107,3 +2211,16 @@ struct torture_suite *torture_smb2_compound_find_init(TALLOC_CTX *ctx) return suite; } + +struct torture_suite *torture_smb2_compound_async_init(TALLOC_CTX *ctx) +{ + struct torture_suite *suite = torture_suite_create(ctx, + "compound_async"); + + torture_suite_add_1smb2_test(suite, "flush_close", + test_compound_async_flush_close); + + suite->description = talloc_strdup(suite, "SMB2-COMPOUND-ASYNC tests"); + + return suite; +} diff --git a/source4/torture/smb2/smb2.c b/source4/torture/smb2/smb2.c index f60477a6c00..0216cdb9ddd 100644 --- a/source4/torture/smb2/smb2.c +++ b/source4/torture/smb2/smb2.c @@ -173,6 +173,7 @@ NTSTATUS torture_smb2_init(TALLOC_CTX *ctx) torture_suite_add_suite(suite, torture_smb2_lease_init(suite)); torture_suite_add_suite(suite, torture_smb2_compound_init(suite)); torture_suite_add_suite(suite, torture_smb2_compound_find_init(suite)); + torture_suite_add_suite(suite, torture_smb2_compound_async_init(suite)); torture_suite_add_suite(suite, torture_smb2_oplocks_init(suite)); torture_suite_add_suite(suite, torture_smb2_kernel_oplocks_init(suite)); torture_suite_add_suite(suite, torture_smb2_streams_init(suite)); -- 2.34.1 From a82db861a583e711701b1f97b7448250e2b3fa39 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 20 Oct 2022 14:22:25 -0700 Subject: [PATCH 2/4] s4: torture: Add an async SMB2_OP_FLUSH + SMB2_OP_FLUSH test to smb2.compound_async. Shows we fail sending an SMB2_OP_FLUSH + SMB2_OP_FLUSH compound if we immediately close the file afterward. Internally the flushes go async and we free the req, then we process the close. When the flushes complete they try to access already freed data. Extra test which will allow me to test when the final component (flush) of the compound goes async and returns NT_STATUS_PENDING. Add knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit 6f149dfd9d8d2619a9e18975ebcf5e69df2b7766) --- selftest/knownfail.d/compound_async | 1 + source4/torture/smb2/compound.c | 115 ++++++++++++++++++++++++++++ 2 files changed, 116 insertions(+) diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async index c18465b7204..e1be97649f3 100644 --- a/selftest/knownfail.d/compound_async +++ b/selftest/knownfail.d/compound_async @@ -1 +1,2 @@ ^samba3.smb2.compound_async.flush_close\(fileserver\) +^samba3.smb2.compound_async.flush_flush\(fileserver\) diff --git a/source4/torture/smb2/compound.c b/source4/torture/smb2/compound.c index e78d78e3a98..47a550f0873 100644 --- a/source4/torture/smb2/compound.c +++ b/source4/torture/smb2/compound.c @@ -2161,6 +2161,119 @@ static bool test_compound_async_flush_close(struct torture_context *tctx, return ret; } +static bool test_compound_async_flush_flush(struct torture_context *tctx, + struct smb2_tree *tree) +{ + struct smb2_handle fhandle = { .data = { 0, 0 } }; + struct smb2_handle relhandle = { .data = { UINT64_MAX, UINT64_MAX } }; + struct smb2_flush fl1; + struct smb2_flush fl2; + const char *fname = "compound_async_flush_flush"; + struct smb2_request *req[2]; + NTSTATUS status; + bool ret = false; + + /* Start clean. */ + smb2_util_unlink(tree, fname); + + /* Create a file. */ + status = torture_smb2_testfile_access(tree, + fname, + &fhandle, + SEC_RIGHTS_FILE_ALL); + CHECK_STATUS(status, NT_STATUS_OK); + + /* Now do a compound flush + flush handle. */ + smb2_transport_compound_start(tree->session->transport, 2); + + ZERO_STRUCT(fl1); + fl1.in.file.handle = fhandle; + + req[0] = smb2_flush_send(tree, &fl1); + torture_assert_not_null_goto(tctx, req[0], ret, done, + "smb2_flush_send (1) failed\n"); + + smb2_transport_compound_set_related(tree->session->transport, true); + + ZERO_STRUCT(fl2); + fl2.in.file.handle = relhandle; + + req[1] = smb2_flush_send(tree, &fl2); + torture_assert_not_null_goto(tctx, req[1], ret, done, + "smb2_flush_send (2) failed\n"); + + status = smb2_flush_recv(req[0], &fl1); + /* + * On Windows, this flush will usually + * succeed as we have nothing to flush, + * so allow NT_STATUS_OK. Once bug #15172 + * is fixed Samba will do the flush synchronously + * so allow NT_STATUS_OK. + */ + if (!NT_STATUS_IS_OK(status)) { + /* + * If we didn't get NT_STATUS_OK, we *must* + * get NT_STATUS_INTERNAL_ERROR if the flush + * goes async. + * + * For pre-bugfix #15172 Samba, the flush goes async and + * we should get NT_STATUS_INTERNAL_ERROR. + */ + torture_assert_ntstatus_equal_goto(tctx, + status, + NT_STATUS_INTERNAL_ERROR, + ret, + done, + "smb2_flush_recv (1) didn't return " + "NT_STATUS_INTERNAL_ERROR.\n"); + } + + /* + * If the flush is the last entry in a compound, + * it should always succeed even if it goes async. + */ + status = smb2_flush_recv(req[1], &fl2); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_flush_recv (2) failed."); + + status = smb2_util_close(tree, fhandle); + torture_assert_ntstatus_ok_goto(tctx, status, ret, done, + "smb2_util_close failed."); + ZERO_STRUCT(fhandle); + + /* + * Do several more operations on the tree, spaced + * out by 1 sec sleeps to make sure the server didn't + * crash on the close. The sleeps are required to + * make test test for a crash reliable, as we ensure + * the pthread fsync internally finishes and accesses + * freed memory. Without them the test occassionally + * passes as we disconnect before the pthread fsync + * finishes. + */ + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OK); + + sleep(1); + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); + + sleep(1); + status = smb2_util_unlink(tree, fname); + CHECK_STATUS(status, NT_STATUS_OBJECT_NAME_NOT_FOUND); + + ret = true; + + done: + + if (fhandle.data[0] != 0) { + smb2_util_close(tree, fhandle); + } + + smb2_util_unlink(tree, fname); + return ret; +} + struct torture_suite *torture_smb2_compound_init(TALLOC_CTX *ctx) { struct torture_suite *suite = torture_suite_create(ctx, "compound"); @@ -2219,6 +2332,8 @@ struct torture_suite *torture_smb2_compound_async_init(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "flush_close", test_compound_async_flush_close); + torture_suite_add_1smb2_test(suite, "flush_flush", + test_compound_async_flush_flush); suite->description = talloc_strdup(suite, "SMB2-COMPOUND-ASYNC tests"); -- 2.34.1 From 60a3c2a567ac964fa72b07c9ffd4bc4ac3abfb33 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 20 Oct 2022 15:08:14 -0700 Subject: [PATCH 3/4] s3: smbd: Add utility function smbd_smb2_is_last_in_compound(). Not yet used. Returns true if we're processing the last SMB2 request in a compound. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme (cherry picked from commit e668c3a82cd566b405c976d45659dd79786948de) --- source3/smbd/globals.h | 1 + source3/smbd/smb2_server.c | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/source3/smbd/globals.h b/source3/smbd/globals.h index eef38f00a4e..efd2032ce1a 100644 --- a/source3/smbd/globals.h +++ b/source3/smbd/globals.h @@ -238,6 +238,7 @@ void smbd_server_disconnect_client_ex(struct smbXsrv_client *client, const char *smb2_opcode_name(uint16_t opcode); bool smbd_is_smb2_header(const uint8_t *inbuf, size_t size); bool smbd_smb2_is_compound(const struct smbd_smb2_request *req); +bool smbd_smb2_is_last_in_compound(const struct smbd_smb2_request *req); NTSTATUS smbd_add_connection(struct smbXsrv_client *client, int sock_fd, NTTIME now, struct smbXsrv_connection **_xconn); diff --git a/source3/smbd/smb2_server.c b/source3/smbd/smb2_server.c index f4e16cb7da9..f0b4814628b 100644 --- a/source3/smbd/smb2_server.c +++ b/source3/smbd/smb2_server.c @@ -229,6 +229,12 @@ bool smbd_smb2_is_compound(const struct smbd_smb2_request *req) return req->in.vector_count >= (2*SMBD_SMB2_NUM_IOV_PER_REQ); } +bool smbd_smb2_is_last_in_compound(const struct smbd_smb2_request *req) +{ + return (req->current_idx + SMBD_SMB2_NUM_IOV_PER_REQ == + req->in.vector_count); +} + static NTSTATUS smbd_initialize_smb2(struct smbXsrv_connection *xconn, uint64_t expected_seq_low) { -- 2.34.1 From 235dedf64b3cbfe38b059ed0c8d393669679fbb4 Mon Sep 17 00:00:00 2001 From: Jeremy Allison Date: Thu, 20 Oct 2022 15:19:05 -0700 Subject: [PATCH 4/4] s3: smbd: Cause SMB2_OP_FLUSH to go synchronous in a compound anywhere but the last operation in the list. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Async read and write go synchronous in the same case, so do the same here. Remove knownfail. BUG: https://bugzilla.samba.org/show_bug.cgi?id=15172 Signed-off-by: Jeremy Allison Reviewed-by: Ralph Boehme Autobuild-User(master): Ralph Böhme Autobuild-Date(master): Thu Nov 17 05:55:42 UTC 2022 on sn-devel-184 (cherry picked from commit 26adf3344337f4e8d5d2107e6ba42e5ea7656372) --- selftest/knownfail.d/compound_async | 2 -- source3/smbd/smb2_flush.c | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 2 deletions(-) delete mode 100644 selftest/knownfail.d/compound_async diff --git a/selftest/knownfail.d/compound_async b/selftest/knownfail.d/compound_async deleted file mode 100644 index e1be97649f3..00000000000 --- a/selftest/knownfail.d/compound_async +++ /dev/null @@ -1,2 +0,0 @@ -^samba3.smb2.compound_async.flush_close\(fileserver\) -^samba3.smb2.compound_async.flush_flush\(fileserver\) diff --git a/source3/smbd/smb2_flush.c b/source3/smbd/smb2_flush.c index e73666f0afc..5c8c171e418 100644 --- a/source3/smbd/smb2_flush.c +++ b/source3/smbd/smb2_flush.c @@ -126,6 +126,8 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, struct tevent_req *subreq; struct smbd_smb2_flush_state *state; struct smb_request *smbreq; + bool is_compound = false; + bool is_last_in_compound = false; req = tevent_req_create(mem_ctx, &state, struct smbd_smb2_flush_state); @@ -195,6 +197,18 @@ static struct tevent_req *smbd_smb2_flush_send(TALLOC_CTX *mem_ctx, tevent_req_set_callback(subreq, smbd_smb2_flush_done, req); + is_compound = smbd_smb2_is_compound(smb2req); + is_last_in_compound = smbd_smb2_is_last_in_compound(smb2req); + + if (is_compound && !is_last_in_compound) { + /* + * Can't go async if we're not the + * last request in a compound request. + * Cause this request to complete synchronously. + */ + smb2_request_set_async_internal(state->smb2req, true); + } + /* Ensure any close request knows about this outstanding IO. */ if (!aio_add_req_to_fsp(fsp, req)) { tevent_req_nterror(req, NT_STATUS_NO_MEMORY); -- 2.34.1