[PATCH v2] ksmb: discard write access to the directory open

[PATCH v2] ksmb: discard write access to the directory open

[Hobin Woo]

may_open() does not allow a directory to be opened with the write access.
However, some writing flags set by client result in adding write access
on server, making ksmbd incompatible with FUSE file system. Simply, let's
discard the write access when opening a directory.

list_add corruption. next is NULL.
------------[ cut here ]------------
kernel BUG at lib/list_debug.c:26!
pc : __list_add_valid+0x88/0xbc
lr : __list_add_valid+0x88/0xbc
Call trace:
__list_add_valid+0x88/0xbc
fuse_finish_open+0x11c/0x170
fuse_open_common+0x284/0x5e8
fuse_dir_open+0x14/0x24
do_dentry_open+0x2a4/0x4e0
dentry_open+0x50/0x80
smb2_open+0xbe4/0x15a4
handle_ksmbd_work+0x478/0x5ec
process_one_work+0x1b4/0x448
worker_thread+0x25c/0x430
kthread+0x104/0x1d4
ret_from_fork+0x10/0x20

Signed-off-by: Yoonho Shin <yoonho.shin@samsung.com>
Signed-off-by: Hobin Woo <hobin.woo@samsung.com>
---
v2:
 - Describe 'is_dir' in function parameters of 'smb2_create_open_flags'

 fs/smb/server/smb2pdu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/fs/smb/server/smb2pdu.c b/fs/smb/server/smb2pdu.c
index e7e07891781b..7d26fdcebbf9 100644
--- a/fs/smb/server/smb2pdu.c
+++ b/fs/smb/server/smb2pdu.c
@@ -2051,15 +2051,22 @@ int smb2_tree_connect(struct ksmbd_work *work)
  * @access:		file access flags
  * @disposition:	file disposition flags
  * @may_flags:		set with MAY_ flags
+ * @is_dir:		is creating open flags for directory
  *
  * Return:      file open flags
  */
 static int smb2_create_open_flags(bool file_present, __le32 access,
 				  __le32 disposition,
-				  int *may_flags)
+				  int *may_flags,
+				  bool is_dir)
 {
 	int oflags = O_NONBLOCK | O_LARGEFILE;
 
+	if (is_dir) {
+		access &= ~FILE_WRITE_DESIRE_ACCESS_LE;
+		ksmbd_debug(SMB, "Discard write access to a directory\n");
+	}
+
 	if (access & FILE_READ_DESIRED_ACCESS_LE &&
 	    access & FILE_WRITE_DESIRE_ACCESS_LE) {
 		oflags |= O_RDWR;
@@ -3167,7 +3174,9 @@ int smb2_open(struct ksmbd_work *work)
 
 	open_flags = smb2_create_open_flags(file_present, daccess,
 					    req->CreateDisposition,
-					    &may_flags);
+					    &may_flags,
+		req->CreateOptions & FILE_DIRECTORY_FILE_LE ||
+		(file_present && S_ISDIR(d_inode(path.dentry)->i_mode)));
 
 	if (!test_tree_conn_flag(tcon, KSMBD_TREE_CONN_FLAG_WRITABLE)) {
 		if (open_flags & (O_CREAT | O_TRUNC)) {
-- 
2.43.0

Re: [PATCH v2] ksmb: discard write access to the directory open

[Namjae Jeon]

2024년 7월 5일 (금) 오후 12:27, Hobin Woo <hobin.woo@samsung.com>님이 작성:
>
> may_open() does not allow a directory to be opened with the write access.
> However, some writing flags set by client result in adding write access
> on server, making ksmbd incompatible with FUSE file system. Simply, let's
> discard the write access when opening a directory.
>
> list_add corruption. next is NULL.
> ------------[ cut here ]------------
> kernel BUG at lib/list_debug.c:26!
> pc : __list_add_valid+0x88/0xbc
> lr : __list_add_valid+0x88/0xbc
> Call trace:
> __list_add_valid+0x88/0xbc
> fuse_finish_open+0x11c/0x170
> fuse_open_common+0x284/0x5e8
> fuse_dir_open+0x14/0x24
> do_dentry_open+0x2a4/0x4e0
> dentry_open+0x50/0x80
> smb2_open+0xbe4/0x15a4
> handle_ksmbd_work+0x478/0x5ec
> process_one_work+0x1b4/0x448
> worker_thread+0x25c/0x430
> kthread+0x104/0x1d4
> ret_from_fork+0x10/0x20
>
> Signed-off-by: Yoonho Shin <yoonho.shin@samsung.com>
> Signed-off-by: Hobin Woo <hobin.woo@samsung.com>
> ---
> v2:
>  - Describe 'is_dir' in function parameters of 'smb2_create_open_flags'
Applied it to #ksmbd-for-next-next.
Thanks for your patch!

Re: [PATCH v2] ksmb: discard write access to the directory open

[Steve French]

fixed typo in commit title and applied to ksmbd-for-next as well

On Fri, Jul 5, 2024 at 6:40 AM Namjae Jeon <linkinjeon@kernel.org> wrote:
>
> 2024년 7월 5일 (금) 오후 12:27, Hobin Woo <hobin.woo@samsung.com>님이 작성:
> >
> > may_open() does not allow a directory to be opened with the write access.
> > However, some writing flags set by client result in adding write access
> > on server, making ksmbd incompatible with FUSE file system. Simply, let's
> > discard the write access when opening a directory.
> >
> > list_add corruption. next is NULL.
> > ------------[ cut here ]------------
> > kernel BUG at lib/list_debug.c:26!
> > pc : __list_add_valid+0x88/0xbc
> > lr : __list_add_valid+0x88/0xbc
> > Call trace:
> > __list_add_valid+0x88/0xbc
> > fuse_finish_open+0x11c/0x170
> > fuse_open_common+0x284/0x5e8
> > fuse_dir_open+0x14/0x24
> > do_dentry_open+0x2a4/0x4e0
> > dentry_open+0x50/0x80
> > smb2_open+0xbe4/0x15a4
> > handle_ksmbd_work+0x478/0x5ec
> > process_one_work+0x1b4/0x448
> > worker_thread+0x25c/0x430
> > kthread+0x104/0x1d4
> > ret_from_fork+0x10/0x20
> >
> > Signed-off-by: Yoonho Shin <yoonho.shin@samsung.com>
> > Signed-off-by: Hobin Woo <hobin.woo@samsung.com>
> > ---
> > v2:
> >  - Describe 'is_dir' in function parameters of 'smb2_create_open_flags'
> Applied it to #ksmbd-for-next-next.
> Thanks for your patch!
>


-- 
Thanks,

Steve

Re: [PATCH v2] ksmb: discard write access to the directory open

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 533 bytes --]

On 7/5/24 5:27 AM, Hobin Woo wrote:
> may_open() does not allow a directory to be opened with the write access.
> However, some writing flags set by client result in adding write access
> on server, making ksmbd incompatible with FUSE file system. Simply, let's
> discard the write access when opening a directory.

afair this should cause a failure like EACCESS or EISDIR and just be 
ignored. What does a Windows server return in this case, or Samba for 
that matter as it (hopefully) will behave like Windows.

-slow


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] ksmb: discard write access to the directory open

[Namjae Jeon]

2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성:
>
> On 7/5/24 5:27 AM, Hobin Woo wrote:
> > may_open() does not allow a directory to be opened with the write access.
> > However, some writing flags set by client result in adding write access
> > on server, making ksmbd incompatible with FUSE file system. Simply, let's
> > discard the write access when opening a directory.
>
> afair this should cause a failure like EACCESS or EISDIR and just be
> ignored. What does a Windows server return in this case, or Samba for
> that matter as it (hopefully) will behave like Windows.
From what I've checked the packet dump, Samba doesn't return any
errors in the same case.
Samba seems to open it with O_RDONLY if it is directory, So there is
no problem, is it right?
>
> -slow
>

Re: [PATCH v2] ksmb: discard write access to the directory open

[Ralph Boehme]

[-- Attachment #1.1: Type: text/plain, Size: 1443 bytes --]

On 7/5/24 2:33 PM, Namjae Jeon wrote:
> 2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성:
>>
>> On 7/5/24 5:27 AM, Hobin Woo wrote:
>>> may_open() does not allow a directory to be opened with the write access.
>>> However, some writing flags set by client result in adding write access
>>> on server, making ksmbd incompatible with FUSE file system. Simply, let's
>>> discard the write access when opening a directory.
>>
>> afair this should cause a failure like EACCESS or EISDIR and just be
>> ignored. What does a Windows server return in this case, or Samba for
>> that matter as it (hopefully) will behave like Windows.
>  From what I've checked the packet dump, Samba doesn't return any
> errors in the same case.
> Samba seems to open it with O_RDONLY if it is directory, So there is
> no problem, is it right?

Hm, it seems my memory is playing tricks on me. Samba indeed forces 
O_RDONLY for directories in the servers and ignores the client requested 
access mask. Interestingly we don't seem to have any test for this case, 
at least I couldn't find any with a quick search. Quickly putting 
together a torture test shows Windows behaves the same. MS-FSA doesn't 
mention any such check should be done for directories as well. Getinfo 
on such a handle even returns the original unmodified client access 
mask, including the write access.

Sorry for the noise! :)

-slow

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] ksmb: discard write access to the directory open

[Tom Talpey]

On 7/5/2024 9:16 AM, Ralph Boehme wrote:
> On 7/5/24 2:33 PM, Namjae Jeon wrote:
>> 2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성:
>>>
>>> On 7/5/24 5:27 AM, Hobin Woo wrote:
>>>> may_open() does not allow a directory to be opened with the write 
>>>> access.
>>>> However, some writing flags set by client result in adding write access
>>>> on server, making ksmbd incompatible with FUSE file system. Simply, 
>>>> let's
>>>> discard the write access when opening a directory.
>>>
>>> afair this should cause a failure like EACCESS or EISDIR and just be
>>> ignored. What does a Windows server return in this case, or Samba for
>>> that matter as it (hopefully) will behave like Windows.
>>  From what I've checked the packet dump, Samba doesn't return any
>> errors in the same case.
>> Samba seems to open it with O_RDONLY if it is directory, So there is
>> no problem, is it right?
> 
> Hm, it seems my memory is playing tricks on me. Samba indeed forces 
> O_RDONLY for directories in the servers and ignores the client requested 
> access mask. Interestingly we don't seem to have any test for this case, 
> at least I couldn't find any with a quick search. Quickly putting 
> together a torture test shows Windows behaves the same. MS-FSA doesn't 
> mention any such check should be done for directories as well. Getinfo 
> on such a handle even returns the original unmodified client access 
> mask, including the write access.
> 
> Sorry for the noise! :)
> 
> -slow

I would ask to see that the SMB3 protocol test suite results are not
impacted by this change, and ideally the various Linux/XFS tests as
well. I don't see any indication these were run?

The other thing I want to point out is that the crash reported in the
commit message is a FUSE failure. Why is that not a bug, and why does
the message not justify why the "fix" is in ksmbd?

Tom.

RE: [PATCH v2] ksmb: discard write access to the directory open

[Hobin Woo]

> On 7/5/2024 9:16 AM, Ralph Boehme wrote:
> > On 7/5/24 2:33 PM, Namjae Jeon wrote:
> >> 2024년 7월 5일 (금) 오후 8:54, Ralph Boehme <slow@samba.org>님이 작성:
> >>>
> >>> On 7/5/24 5:27 AM, Hobin Woo wrote:
> >>>> may_open() does not allow a directory to be opened with the write
> >>>> access.
> >>>> However, some writing flags set by client result in adding write
> access
> >>>> on server, making ksmbd incompatible with FUSE file system. Simply,
> >>>> let's
> >>>> discard the write access when opening a directory.
> >>>
> >>> afair this should cause a failure like EACCESS or EISDIR and just be
> >>> ignored. What does a Windows server return in this case, or Samba for
> >>> that matter as it (hopefully) will behave like Windows.
> >>  From what I've checked the packet dump, Samba doesn't return any
> >> errors in the same case.
> >> Samba seems to open it with O_RDONLY if it is directory, So there is
> >> no problem, is it right?
> >
> > Hm, it seems my memory is playing tricks on me. Samba indeed forces
> > O_RDONLY for directories in the servers and ignores the client
> requested
> > access mask. Interestingly we don't seem to have any test for this case,
> > at least I couldn't find any with a quick search. Quickly putting
> > together a torture test shows Windows behaves the same. MS-FSA doesn't
> > mention any such check should be done for directories as well. Getinfo
> > on such a handle even returns the original unmodified client access
> > mask, including the write access.
Right. That is why I fixed ksmd not FUSE.
> >
> > Sorry for the noise! :)
> >
> > -slow
> 
> I would ask to see that the SMB3 protocol test suite results are not
> impacted by this change, and ideally the various Linux/XFS tests as
> well. I don't see any indication these were run?
> 
> The other thing I want to point out is that the crash reported in the
> commit message is a FUSE failure. Why is that not a bug, and why does
> the message not justify why the "fix" is in ksmbd?
As you pointed out, if FUSE had additional checks for this, then the issue
wouldn't have occurred in the first place. However, I believe it is not the
fundamental fix since FUSE operates correctly under the sanity checks of VFS. 

Widely used file systems may perform such checks by themselves or might not 
have the functionality related to this. But if any new functionality
relevant to this were added, we can expect similar problems to happen.

Hobin
> 
> Tom.