> >> 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
