From ef8b7311e718b47af46cd547842851a1dd164709 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Mon, 6 Aug 2018 14:33:34 +0200 Subject: [PATCH 1/2] vfs_fruit: Fix a leak of "br_lck" Fix a panic if fruit_access_check detects a locking conflict. do_lock() returns a valid br_lck even in case of a locking conflict. Not free'ing it leads to a invalid lock order panic later, because "br_lck" corresponds to a dbwrap lock on brlock.tdb. BUG: https://bugzilla.samba.org/show_bug.cgi?id=13584 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison (cherry picked from commit 51d57073798f76ec4f1261945e0ba779b2530009) --- source3/modules/vfs_fruit.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/source3/modules/vfs_fruit.c b/source3/modules/vfs_fruit.c index 078426290a4..ebf0f9842f0 100644 --- a/source3/modules/vfs_fruit.c +++ b/source3/modules/vfs_fruit.c @@ -2386,7 +2386,6 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, uint32_t deny_mode) { NTSTATUS status = NT_STATUS_OK; - struct byte_range_lock *br_lck = NULL; bool open_for_reading, open_for_writing, deny_read, deny_write; off_t off; bool have_read = false; @@ -2444,6 +2443,8 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, /* Set locks */ if ((access_mask & FILE_READ_DATA) && have_read) { + struct byte_range_lock *br_lck = NULL; + off = access_to_netatalk_brl(fork_type, FILE_READ_DATA); br_lck = do_lock( handle->conn->sconn->msg_ctx, fsp, @@ -2451,13 +2452,16 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, READ_LOCK, POSIX_LOCK, false, &status, NULL); + TALLOC_FREE(br_lck); + if (!NT_STATUS_IS_OK(status)) { return status; } - TALLOC_FREE(br_lck); } if ((deny_mode & DENY_READ) && have_read) { + struct byte_range_lock *br_lck = NULL; + off = denymode_to_netatalk_brl(fork_type, DENY_READ); br_lck = do_lock( handle->conn->sconn->msg_ctx, fsp, @@ -2465,10 +2469,11 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, READ_LOCK, POSIX_LOCK, false, &status, NULL); + TALLOC_FREE(br_lck); + if (!NT_STATUS_IS_OK(status)) { return status; } - TALLOC_FREE(br_lck); } } @@ -2494,6 +2499,8 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, /* Set locks */ if ((access_mask & FILE_WRITE_DATA) && have_read) { + struct byte_range_lock *br_lck = NULL; + off = access_to_netatalk_brl(fork_type, FILE_WRITE_DATA); br_lck = do_lock( handle->conn->sconn->msg_ctx, fsp, @@ -2501,13 +2508,15 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, READ_LOCK, POSIX_LOCK, false, &status, NULL); + TALLOC_FREE(br_lck); + if (!NT_STATUS_IS_OK(status)) { return status; } - TALLOC_FREE(br_lck); - } if ((deny_mode & DENY_WRITE) && have_read) { + struct byte_range_lock *br_lck = NULL; + off = denymode_to_netatalk_brl(fork_type, DENY_WRITE); br_lck = do_lock( handle->conn->sconn->msg_ctx, fsp, @@ -2515,15 +2524,14 @@ static NTSTATUS fruit_check_access(vfs_handle_struct *handle, READ_LOCK, POSIX_LOCK, false, &status, NULL); + TALLOC_FREE(br_lck); + if (!NT_STATUS_IS_OK(status)) { return status; } - TALLOC_FREE(br_lck); } } - TALLOC_FREE(br_lck); - return status; } -- 2.18.0.1017.ga543ac7ca45-goog From bda2aa8cc30df90e625018dd224d1cbae5cc6705 Mon Sep 17 00:00:00 2001 From: Volker Lendecke Date: Mon, 6 Aug 2018 14:35:15 +0200 Subject: [PATCH 2/2] torture: Demonstrate the invalid lock order panic BUG: https://bugzilla.samba.org/show_bug.cgi?id=13584 Signed-off-by: Volker Lendecke Reviewed-by: Jeremy Allison Autobuild-User(master): Jeremy Allison Autobuild-Date(master): Tue Aug 21 02:33:05 CEST 2018 on sn-devel-144 (cherry picked from commit ec3c37ee53f21d8c0e80b1d3b3d7e95a4ac8e0bc) --- source4/torture/vfs/fruit.c | 89 +++++++++++++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) diff --git a/source4/torture/vfs/fruit.c b/source4/torture/vfs/fruit.c index 4c49a6bf532..fc1760e02e0 100644 --- a/source4/torture/vfs/fruit.c +++ b/source4/torture/vfs/fruit.c @@ -4891,6 +4891,93 @@ done: return ret; } +static bool test_fruit_locking_conflict(struct torture_context *tctx, + struct smb2_tree *tree) +{ + TALLOC_CTX *mem_ctx; + struct smb2_create create; + struct smb2_handle h; + struct smb2_lock lck; + struct smb2_lock_element el; + const char *fname = BASEDIR "\\locking_conflict.txt"; + NTSTATUS status; + bool ret = false; + + mem_ctx = talloc_new(tctx); + torture_assert_not_null(tctx, mem_ctx, "talloc_new failed"); + + /* clean slate ...*/ + smb2_util_unlink(tree, fname); + smb2_deltree(tree, fname); + smb2_deltree(tree, BASEDIR); + + status = torture_smb2_testdir(tree, BASEDIR, &h); + CHECK_STATUS(status, NT_STATUS_OK); + smb2_util_close(tree, h); + + create = (struct smb2_create) { + .in.desired_access = SEC_RIGHTS_FILE_READ, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = + NTCREATEX_SHARE_ACCESS_READ| + NTCREATEX_SHARE_ACCESS_WRITE, + .in.create_disposition = NTCREATEX_DISP_CREATE, + .in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, mem_ctx, &create); + CHECK_STATUS(status, NT_STATUS_OK); + h = create.out.file.handle; + + el = (struct smb2_lock_element) { + .offset = 0xfffffffffffffffc, + .length = 1, + .flags = SMB2_LOCK_FLAG_EXCLUSIVE, + }; + lck = (struct smb2_lock) { + .in.lock_count = 1, + .in.file.handle = h, + .in.locks = &el, + }; + + status = smb2_lock(tree, &lck); + CHECK_STATUS(status, NT_STATUS_OK); + + el = (struct smb2_lock_element) { + .offset = 0, + .length = 0x7fffffffffffffff, + .flags = SMB2_LOCK_FLAG_EXCLUSIVE, + }; + status = smb2_lock(tree, &lck); + CHECK_STATUS(status, NT_STATUS_OK); + + create = (struct smb2_create) { + .in.desired_access = + SEC_RIGHTS_FILE_READ|SEC_RIGHTS_FILE_WRITE, + .in.file_attributes = FILE_ATTRIBUTE_NORMAL, + .in.share_access = NTCREATEX_SHARE_ACCESS_READ, + .in.create_disposition = NTCREATEX_DISP_OPEN, + .in.impersonation_level = SMB2_IMPERSONATION_ANONYMOUS, + .in.fname = fname, + }; + + status = smb2_create(tree, mem_ctx, &create); + CHECK_STATUS(status, NT_STATUS_FILE_LOCK_CONFLICT); + + { + struct smb2_close cl = { + .level = RAW_CLOSE_SMB2, + .in.file.handle = h, + }; + smb2_close(tree, &cl); + } + + ret = true; +done: + return ret; +} + struct torture_suite *torture_vfs_fruit_netatalk(TALLOC_CTX *ctx) { struct torture_suite *suite = torture_suite_create( @@ -4900,6 +4987,8 @@ struct torture_suite *torture_vfs_fruit_netatalk(TALLOC_CTX *ctx) torture_suite_add_1smb2_test(suite, "read netatalk metadata", test_read_netatalk_metadata); torture_suite_add_1smb2_test(suite, "stream names with locally created xattr", test_stream_names_local); + torture_suite_add_1smb2_test( + suite, "locking conflict", test_fruit_locking_conflict); return suite; } -- 2.18.0.1017.ga543ac7ca45-goog