[PATCH 0/2] ksmbd: exclude metadata-only opens from sharing checks

[PATCH 0/2] ksmbd: exclude metadata-only opens from sharing checks

[Ian Bridges]

A metadata-only open holds none of FILE_EXECUTE, FILE_READ_DATA,
FILE_WRITE_DATA, FILE_APPEND_DATA and DELETE. An open for READ_CONTROL
is one example. ksmbd still counts such opens in sharing checks. Held
without sharing, one blocks later reads, writes and deletes of the file.
Held on a stream without FILE_SHARE_DELETE, one blocks deleting the
file.

Patch 1 fixes the general sharing check. Patch 2 fixes the stream
delete check added in v7.2. They are split so that patch 1 can be
backported without the v7.2 stream helper.

This series was developed with LLM assistance.

Ian Bridges (2):
  ksmbd: skip metadata-only opens in sharing checks
  ksmbd: skip metadata-only stream opens in the delete check

 fs/smb/server/smb_common.c | 9 +++++++++
 fs/smb/server/smb_common.h | 4 ++++
 fs/smb/server/vfs_cache.c  | 7 ++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)


base-commit: e49296d51ccbf2ad0ed0484db1382f1d7217bd6e
-- 
2.47.3

[PATCH 1/2] ksmbd: skip metadata-only opens in sharing checks

[Ian Bridges]

A READ_CONTROL open without FILE_SHARE_DELETE makes a later DELETE open
of the file fail with STATUS_SHARING_VIOLATION.

ksmbd_smb_check_shared_mode() compares the sharing modes of two opens
even when one holds none of FILE_EXECUTE, FILE_READ_DATA,
FILE_WRITE_DATA, FILE_APPEND_DATA and DELETE. It skips a pair only when
exactly one of them asks for nothing but attribute rights.

Skip a pair of opens unless both hold one of those rights.

Fixes: e2f34481b24d ("cifsd: add server-side procedures for SMB3")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---

Notes:
    An authenticated user can trigger it on any share, and a guest can on a
    share with "guest ok = yes". On a share without stored DACLs an open for
    FILE_READ_ATTRIBUTES and READ_CONTROL needs only search permission on
    the directory. A user with that permission can hold such an open without
    sharing and block the owner's reads, writes and deletes of a file in it.
    
    Tested on ksmbd-for-next e49296d51ccb with KASAN, with no reports or
    warnings. In targeted sharing tests and a KUnit matrix every changed
    result removes a sharing violation that MS-FSA does not make.
    smbtorture gives the same results as on the base.
    
    A reproducer is available on request.
    
    This fix was developed with LLM assistance.

 fs/smb/server/smb_common.c | 9 +++++++++
 fs/smb/server/smb_common.h | 4 ++++
 2 files changed, 13 insertions(+)

diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c
index f9fb3741ada3..920e94017bf6 100644
--- a/fs/smb/server/smb_common.c
+++ b/fs/smb/server/smb_common.c
@@ -719,6 +719,15 @@ int ksmbd_smb_check_shared_mode(struct file *filp, struct ksmbd_file *curr_fp)
 		if (prev_fp->attrib_only != curr_fp->attrib_only)
 			continue;
 
+		/*
+		 * Two opens conflict only if both hold a right in
+		 * FILE_SHARE_CHECKED_ACCESS_LE (MS-FSA 2.1.5.1.2.1 and
+		 * 2.1.5.1.2.2)
+		 */
+		if (!(prev_fp->daccess & FILE_SHARE_CHECKED_ACCESS_LE) ||
+		    !(curr_fp->daccess & FILE_SHARE_CHECKED_ACCESS_LE))
+			continue;
+
 		if (!(prev_fp->saccess & FILE_SHARE_DELETE_LE) &&
 		    curr_fp->daccess & FILE_DELETE_LE) {
 			smb_shared_mode_error(SHARE_DELETE_ERROR,
diff --git a/fs/smb/server/smb_common.h b/fs/smb/server/smb_common.h
index a9cc09e31721..4d404d1592c6 100644
--- a/fs/smb/server/smb_common.h
+++ b/fs/smb/server/smb_common.h
@@ -69,6 +69,10 @@
 		FILE_EXECUTE | FILE_DELETE_CHILD | \
 		FILE_READ_ATTRIBUTES | FILE_WRITE_ATTRIBUTES)
 
+/* Rights that make an open take part in sharing checks */
+#define FILE_SHARE_CHECKED_ACCESS_LE	(FILE_EXECUTE_LE | FILE_READ_DATA_LE | \
+		FILE_WRITE_DATA_LE | FILE_APPEND_DATA_LE | FILE_DELETE_LE)
+
 #define SMB_COM_NEGOTIATE		0x72 /* See MS-CIFS 2.2.2.1 */
 
 /* See MS-CIFS 2.2.3.1 */
-- 
2.47.3

[PATCH 2/2] ksmbd: skip metadata-only stream opens in the delete check

[Ian Bridges]

A READ_CONTROL open of a named stream without FILE_SHARE_DELETE makes a
DELETE open of the file fail with STATUS_SHARING_VIOLATION. Setting the
file delete pending fails the same way.

ksmbd_has_stream_without_delete_share() counts every stream open without
FILE_SHARE_DELETE, whatever access it holds.

Count a stream open only when it holds FILE_EXECUTE, FILE_READ_DATA,
FILE_WRITE_DATA, FILE_APPEND_DATA or DELETE.

Fixes: 19043971c947 ("ksmbd: honor stream delete sharing for base file")
Cc: stable@vger.kernel.org
Signed-off-by: Ian Bridges <icb@fastmail.org>
---

Notes:
    Any share with "vfs objects = streams_xattr" is affected. A READ_CONTROL
    stream open without FILE_SHARE_DELETE blocks every DELETE open of the
    file and setting it delete pending.
    
    Tested on ksmbd-for-next e49296d51ccb with KASAN, with no reports or
    warnings. In targeted stream tests and a KUnit matrix every changed
    result removes a sharing violation that MS-FSA does not make.
    smbtorture gives the same results as on the base.
    
    A reproducer is available on request.
    
    This fix was developed with LLM assistance.

 fs/smb/server/vfs_cache.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/fs/smb/server/vfs_cache.c b/fs/smb/server/vfs_cache.c
index f9079c529d07..6675c6652c13 100644
--- a/fs/smb/server/vfs_cache.c
+++ b/fs/smb/server/vfs_cache.c
@@ -420,7 +420,12 @@ bool ksmbd_has_stream_without_delete_share(struct ksmbd_file *fp)
 		if (file_inode(fp->filp) != file_inode(prev_fp->filp))
 			continue;
 
-		if (!(prev_fp->saccess & FILE_SHARE_DELETE_LE)) {
+		/*
+		 * A stream open with no right in FILE_SHARE_CHECKED_ACCESS_LE
+		 * never conflicts
+		 */
+		if (!(prev_fp->saccess & FILE_SHARE_DELETE_LE) &&
+		    prev_fp->daccess & FILE_SHARE_CHECKED_ACCESS_LE) {
 			ret = true;
 			break;
 		}
-- 
2.47.3

Re: [PATCH 1/2] ksmbd: skip metadata-only opens in sharing checks

[Namjae Jeon]

> diff --git a/fs/smb/server/smb_common.c b/fs/smb/server/smb_common.c
> index f9fb3741ada3..920e94017bf6 100644
> --- a/fs/smb/server/smb_common.c
> +++ b/fs/smb/server/smb_common.c
> @@ -719,6 +719,15 @@ int ksmbd_smb_check_shared_mode(struct file *filp, struct ksmbd_file *curr_fp)
>                 if (prev_fp->attrib_only != curr_fp->attrib_only)
>                         continue;
>
> +               /*
> +                * Two opens conflict only if both hold a right in
> +                * FILE_SHARE_CHECKED_ACCESS_LE (MS-FSA 2.1.5.1.2.1 and
> +                * 2.1.5.1.2.2)
> +                */
> +               if (!(prev_fp->daccess & FILE_SHARE_CHECKED_ACCESS_LE) ||
> +                   !(curr_fp->daccess & FILE_SHARE_CHECKED_ACCESS_LE))
> +                       continue;
> +
Could we account for O_TRUNC in the access mask used for sharing
checks before adding this shortcut ?  CREATE requesting READ_CONTROL
can still specify FILE_OVERWRITE. smb2_create_open_flags() then sets
O_TRUNC, but fp->daccess remains READ_CONTROL, so this condition skips
the sharing checks even though the request can reach
smb2_create_truncate(). For example, with an existing FILE_READ_DATA
open and ShareAccess=0, second CREATE specifying READ_CONTROL,
FILE_OVERWRITE and ShareAccess=0 changes the sharing-check result from
-EPERM to success in the extracted-function harness. The open should
be checked using an effective access mask that includes
FILE_WRITE_DATA when O_TRUNC is requested. The client's requested
access can be kept separately for the returned handle. This would let
the shortcut exclude opens that only access metadata while still
checking opens that truncate file data.

Thanks.

Re: [PATCH 1/2] ksmbd: skip metadata-only opens in sharing checks

[Ian Bridges]

Hi Namjae,

You're right. With this patch a CREATE for READ_CONTROL with
FILE_OVERWRITE or FILE_SUPERSEDE skips the sharing check, and
smb2_open() then truncates the file. In your example, with an existing
FILE_READ_DATA open at ShareAccess 0, the second CREATE for
READ_CONTROL and FILE_OVERWRITE at ShareAccess 0 succeeds and the
file's data is lost. A test that runs smb2_open() confirms it.

I'll send a v2 that does what you suggest: the sharing check counts an
open that truncates the file as holding FILE_WRITE_DATA, and the
returned handle keeps the open's own access. An open of a directory or
a named stream, or one for attributes only, gets O_TRUNC but is not
truncated, so it is not counted.

A truncating open is already on the inode's open list before it
truncates, so v2 also counts it as holding FILE_WRITE_DATA, for as long
as it is open, when deciding whether a later open is checked against
it. Patch 2 does not change.

Thanks,
Ian