RE: updated ksmbd (cifsd)

["Namjae Jeon" <namjae.jeon@samsung.com>]

> >> 2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ?
> > Because of smb2.acls.GENERIC failure.
> >
> > TESTING FILE GENERIC BITS
> > get the original sd
> > Testing generic bits 0x00000000
> > time: 2020-12-15 00:00:37.940992
> > failure: GENERIC [
> > (../../source4/torture/smb2/acls.c:439) Incorrect status
> > NT_STATUS_SHARING_VIOLATION - should be NT_STATUS_OK
> >
> > I really don't understand this test. This testcase expect that
> > FILE_DELETE is set in response if desired access of smb2 open is FILE_MAXIMAL_ACCESS.
> > I don't know why smb2 open should be allowed although FILE_SHARE_DELETE is not set in previous open
> in this test.
> > Can you give me a hint ?
> 
> As far as I can see the test assumes the user has SeRestorePrivilege, with that
> SEC_FLAG_MAXIMUM_ALLOWED will add FILE_DELETE, see https://protect2.fireeye.com/v1/url?k=3a9ae45d-
> 6501dd47-3a9b6f12-000babff24ad-8398dba5a818cd4a&q=1&e=bdf5897b-3ecc-49dc-9105-
> 2d6782854fcc&u=https%3A%2F%2Fdocs.microsoft.com%2Fen-us%2Fopenspecs%2Fwindows_protocols%2Fms-
> fsa%2F8ada5fbe-db4e-49fd-aef6-20d54b748e40
The question I'm asking is how it can be opened with FILE DELETE that adding
by SEC_FLAG_MAXIMUM_ALLOWED without FILE_SHARE_DELETE in 1st open.
NT_STATUS_SHARING_VIOLATION error should be returned? but this test should be allowed to open.

It test in the following sequences.
- 1st smb2 open with NTCREATEX_SHARE_ACCESS_READ | NTCREATEX_SHARE_ACCESS_WRITE
- SMB2 set info security().
- 2nd open with SEC_FLAG_MAXIMUM_ALLOWED(adding FILE DELETE) => NT_STATUS_SHARING_VIOLATION or NT_STATUS_OK ?

> 
> >> 3. Why does ksmbd_override_fsids() only reset cred->fs[g|u]id, but group_info
> >>    is kept unchanged, I guess at least the groups array should be set to be empty.
> > Yes, We need to handle the groups list. Will fix it.
> >
> >> 4. What is work->saved_cred_level used for in ksmbd_override_fsids()?
> >>    It seems to be unused and adds a lot of complexity.
> > ksmbd_override_fsids could be called recursively.
> > work->saved_cred_level prevents ksmbd from overriding fs[g|u]id again.
> 
> But that will always be on the same session/share combination?
Ah, ksmbd_override_fsids() has been recursively called to handle SMB1 requests.
At present, SMB1 codes was removed in smb3_kernel tree, So we can remove work->saved_cred_level.

Thanks for your review!
> 
> >> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references
> https://protect2.fireeye.com/v1/url?k=6f3cad54-30a7944e-6f3d261b-000babff24ad-
> 32002aad36f8cca9&q=1&e=bdf5897b-3ecc-49dc-9105-2d6782854fcc&u=https%3A%2F%2Fgithub.com%2Fcifsd-
> team%2Fcifsd-tools
> >>   instead of
> >> https://protect2.fireeye.com/v1/url?k=cf0932a6-90920bbc-cf08b9e9-000b
> >> abff24ad-ea69fcf05590fae2&q=1&e=bdf5897b-3ecc-49dc-9105-2d6782854fcc&
> >> u=https%3A%2F%2Fgithub.com%2Fcifsd-team%2Fksmbd-tools
> > Okay. Will update it.
> 
> Thanks!
> 
> >> 6. Why is SMB_SERVER_CHECK_CAP_NET_ADMIN an compile time option and why is it off by default?
> >>    I think the behavior should be enforced without a switch.
> > I can make it default yes. Can you explain more why it should be enforced ?
> 
> Why should an unprivileged user ever be able to start the server?
> Wouldn't that be a massive security problem as that user would provide the share definitions and users
> and controls what ksmbd_override_fsids() will use?
> 
> metze