updated ksmbd (cifsd)

updated ksmbd (cifsd)

[Steve French]

I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
results (and recent improvements) running Linux cifs.ko->ksmbd look
very promising.



-- 
Thanks,

Steve

Re: updated ksmbd (cifsd)

[Namjae Jeon]

2020-12-14 10:20 GMT+09:00, Steve French via samba-technical
<samba-technical@lists.samba.org>:
> I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
> ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
> results (and recent improvements) running Linux cifs.ko->ksmbd look
> very promising.
With the review of Ronnie, I fixed one problem in previous patch. I have sent
a pull request for this patch. And I am running the number of 118 tests
of xfstests on rebased smb3-kernel(+ the patch in pull request) again.
I will share the result as soon as it is finished.
Thanks!
>
>
>
> --
> Thanks,
>
> Steve
>
>

Re: updated ksmbd (cifsd)

[Stefan Metzmacher]

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

Am 14.12.20 um 02:20 schrieb Steve French via samba-technical:
> I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
> ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
> results (and recent improvements) running Linux cifs.ko->ksmbd look
> very promising.

I just looked briefly, but I'm wondering about a few things:

1. The xattr's to store additional meta data are not compatible with
   Samba's way of storing things:
   https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl

   In order to make it possible to use the same filesystem with both servers
   it would be great if the well established way used in Samba would be used
   as well.

2. Why does smb2_set_info_sec() have fp->saccess |= FILE_SHARE_DELETE_LE; ?

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.

4. What is work->saved_cred_level used for in ksmbd_override_fsids()?
   It seems to be unused and adds a lot of complexity.

5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools
   instead of https://github.com/cifsd-team/ksmbd-tools

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.

These are just some initial thoughts.

metze


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

Re: updated ksmbd (cifsd)

[Jeremy Allison]

On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote:
>Am 14.12.20 um 02:20 schrieb Steve French via samba-technical:
>> I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
>> ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
>> results (and recent improvements) running Linux cifs.ko->ksmbd look
>> very promising.
>
>I just looked briefly, but I'm wondering about a few things:
>
>1. The xattr's to store additional meta data are not compatible with
>   Samba's way of storing things:
>   https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
>
>   In order to make it possible to use the same filesystem with both servers
>   it would be great if the well established way used in Samba would be used
>   as well.

A thousand times this ! If cifs.ko->ksmbd adds a differnt way
of storing the extra meta-data that is incompatible with Samba
this would be a disaster for users.

Please fix this before proposing any merge.

RE: updated ksmbd (cifsd)

[Namjae Jeon]

> > I just rebased https://github.com/smfrench/smb3-kernel/tree/cifsd-for-next
> > ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
> > results (and recent improvements) running Linux cifs.ko->ksmbd look
> > very promising.
> With the help of Ronnie, I fixed one problem in previous patch. I have sent
> a pull request for this patch. And I have also checked number of 118 tests of
> xfstests were passed on rebased smb3-kernel(+ the patch in pull request).

I share the xfstests's result of 5.10 kernel+ksmbd(cifsd).
---------------------->8------------------
FSTYP         -- cifs
PLATFORM      -- Linux/x86_64 linkinjeon-Samsung-DeskTop-System 5.10.0+ #1 SMP Mon Dec 14 10:55:27 KST 2020
MKFS_OPTIONS  -- //10.88.103.87/homes
MOUNT_OPTIONS -- -ousername=linkinjeon,password=1234,noperm,vers=3.11,mfsymlinks,nostrictsync //10.88.103.87/homes /mnt/test

cifs/001 files ...  1s
generic/001 files ...  15s
generic/002 files ...  2s
generic/005 files ...  2s
generic/006 files ...  58s
generic/007 files ...  46s
generic/008 files ...  1s
generic/011 files ...  56s
generic/013 files ...  62s
generic/014 files ...  12s
generic/020 files ...  3s
generic/023 files ...  2s
generic/024 files ...  2s
generic/028 files ...  6s
generic/029 files ...  2s
generic/030 files ...  1s
generic/032 files ...  14s
generic/033 files ...  1s
generic/036 files ...  11s
generic/037 files ...  9s
generic/069 files ...  18s
generic/070 files ...  39s
generic/071 files ...  1s
generic/074 files ...  98s
generic/080 files ...  3s
generic/084 files ...  6s
generic/086 files ...  2s
generic/095 files ...  6s
generic/098 files ...  2s
generic/100 files ...  39s
generic/103 files ...  2s
generic/109 files ...  28s
generic/113 files ...  7s
generic/117 files ...  38s
generic/124 files ...  7s
generic/125 files ...  62s
generic/129 files ...  35s
generic/130 files ...  4s
generic/132 files ...  16s
generic/133 files ...  36s
generic/135 files ...  1s
generic/141 files ...  1s
generic/169 files ...  1s
generic/198 files ...  1s
generic/207 files ...  2s
generic/208 files ...  200s
generic/210 files ...  0s
generic/211 files ...  0s
generic/212 files ...  0s
generic/214 files ...  1s
generic/215 files ...  3s
generic/221 files ...  2s
generic/225 files ...  9s
generic/228 files ...  1s
generic/236 files ...  2s
generic/239 files ...  31s
generic/241 files ...  73s
generic/245 files ...  0s
generic/246 files ...  1s
generic/247 files ...  20s
generic/248 files ...  0s
generic/249 files ...  2s
generic/257 files ...  2s
generic/258 files ...  1s
generic/286 files ...  6s
generic/308 files ...  1s
generic/309 files ...  2s
generic/310 files ...  101s
generic/313 files ...  4s
generic/315 files ...  1s
generic/316 files ...  2s
generic/337 files ...  0s
generic/339 files ...  4s
generic/340 files ...  6s
generic/344 files ...  11s
generic/345 files ...  11s
generic/346 files ...  25s
generic/349 files ...  4s
generic/350 files ...  3s
generic/354 files ...  9s
generic/360 files ...  0s
generic/377 files ...  1s
generic/391 files ...  10s
generic/393 files ...  3s
generic/394 files ...  2s
generic/406 files ...  2s
generic/412 files ...  1s
generic/420 files ...  1s
generic/422 files ...  2s
generic/428 files ...  1s
generic/432 files ...  1s
generic/433 files ...  1s
generic/436 files ...  2s
generic/437 files ...  2s
generic/438 files ...  69s
generic/439 files ...  1s
generic/443 files ...  1s
generic/445 files ...  2s
generic/446 files ...  14s
generic/448 files ...  3s
generic/451 files ...  31s
generic/452 files ...  1s
generic/454 files ...  2s
generic/460 files ...  9s
generic/464 files ...  54s
generic/465 files ...  3s
generic/476 files ...  18864s
generic/490 files ...  3s
generic/504 files ...  0s
generic/523 files ...  118s
generic/524 files ...  6s
generic/533 files ...  2s
generic/539 files ...  2s
generic/551 files ...  7923s
generic/567 files ...  4s
generic/568 files ...  1s
generic/590 files ...  106s
generic/591 files ...  1s
Ran: cifs/001 generic/001 generic/002 generic/005 generic/006 generic/007 generic/008 generic/011 generic/013 generic/014 generic/020 generic/023 generic/024 generic/028 generic/029 generic/030 generic/032 generic/033 generic/036 generic/037 generic/069 generic/070 generic/071 generic/074 generic/080 generic/084 generic/086 generic/095 generic/098 generic/100 generic/103 generic/109 generic/113 generic/117 generic/124 generic/125 generic/129 generic/130 generic/132 generic/133 generic/135 generic/141 generic/169 generic/198 generic/207 generic/208 generic/210 generic/211 generic/212 generic/214 generic/215 generic/221 generic/225 generic/228 generic/236 generic/239 generic/241 generic/245 generic/246 generic/247 generic/248 generic/249 generic/257 generic/258 generic/286 generic/308 generic/309 generic/310 generic/313 generic/315 generic/316 generic/337 generic/339 generic/340 generic/344 generic/345 generic/346 generic/349 generic/350 generic/354 generic/360 generic/377 generic/391 generic/393 generic/394 generic/406 generic/412 generic/420 generic/422 generic/428 generic/432 generic/433 generic/436 generic/437 generic/438 generic/439 generic/443 generic/445 generic/446 generic/448 generic/451 generic/452 generic/454 generic/460 generic/464 generic/465 generic/476 generic/490 generic/504 generic/523 generic/524 generic/533 generic/539 generic/551 generic/567 generic/568 generic/590 generic/591
Passed all 118 tests

RE: updated ksmbd (cifsd)

[Namjae Jeon]

 
> I just looked briefly, but I'm wondering about a few things:
> 
> 1. The xattr's to store additional meta data are not compatible with
>    Samba's way of storing things:
>     https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
> 
>    In order to make it possible to use the same filesystem with both servers
>    it would be great if the well established way used in Samba would be used
>    as well.
Yes, Maybe... I will check it after sending ksmbd to linux-next.
 
> 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 ?

> 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.
 
> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools
>   instead of https://github.com/cifsd-team/ksmbd-tools
Okay. Will update it.

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

> These are just some initial thoughts.
Thanks for sharing your review!
> 
> metze

RE: updated ksmbd (cifsd)

[Namjae Jeon]

> On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote:
> >Am 14.12.20 um 02:20 schrieb Steve French via samba-technical:
> >> I just rebased https://protect2.fireeye.com/v1/url?k=e100f21c-be9bcb17-e1017953-002590f5b904-
> f00629b46b3afee4&q=1&e=6fc8b980-0fd2-4e4d-a9dc-
> 9ea15e482833&u=https%3A%2F%2Fgithub.com%2Fsmfrench%2Fsmb3-kernel%2Ftree%2Fcifsd-for-next
> >> ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
> >> results (and recent improvements) running Linux cifs.ko->ksmbd look
> >> very promising.
> >
> >I just looked briefly, but I'm wondering about a few things:
> >
> >1. The xattr's to store additional meta data are not compatible with
> >   Samba's way of storing things:
> >
> >https://protect2.fireeye.com/v1/url?k=fbb13e03-a42a0708-fbb0b54c-002590
> >f5b904-f4288e37b0eb9ae8&q=1&e=6fc8b980-0fd2-4e4d-a9dc-9ea15e482833&u=ht
> >tps%3A%2F%2Fgit.samba.org%2F%3Fp%3Dsamba.git%3Ba%3Dblob%3Bf%3Dlibrpc%2F
> >idl%2Fxattr.idl
> >
> >   In order to make it possible to use the same filesystem with both servers
> >   it would be great if the well established way used in Samba would be used
> >   as well.
> 
> A thousand times this ! If cifs.ko->ksmbd adds a differnt way of storing the extra meta-data that is
> incompatible with Samba this would be a disaster for users.
> 
> Please fix this before proposing any merge.
You said that samba can handle it even if ksmbd has own extra metadata format. I didn't think it was
necessary to what you said. If we have to do this, I think it is not too late to work after sending
ksmbd to linux-next first.

Re: updated ksmbd (cifsd)

[Jeremy Allison]

On Tue, Dec 15, 2020 at 11:29:01AM +0900, Namjae Jeon via samba-technical wrote:
>
>> On Mon, Dec 14, 2020 at 06:45:51PM +0100, Stefan Metzmacher via samba-technical wrote:
>> >Am 14.12.20 um 02:20 schrieb Steve French via samba-technical:
>> >> I just rebased https://protect2.fireeye.com/v1/url?k=e100f21c-be9bcb17-e1017953-002590f5b904-
>> f00629b46b3afee4&q=1&e=6fc8b980-0fd2-4e4d-a9dc-
>> 9ea15e482833&u=https%3A%2F%2Fgithub.com%2Fsmfrench%2Fsmb3-kernel%2Ftree%2Fcifsd-for-next
>> >> ontop of 5.10 kernel. Let me know if you see any problems.   xfstest
>> >> results (and recent improvements) running Linux cifs.ko->ksmbd look
>> >> very promising.
>> >
>> >I just looked briefly, but I'm wondering about a few things:
>> >
>> >1. The xattr's to store additional meta data are not compatible with
>> >   Samba's way of storing things:
>> >
>> >https://protect2.fireeye.com/v1/url?k=fbb13e03-a42a0708-fbb0b54c-002590
>> >f5b904-f4288e37b0eb9ae8&q=1&e=6fc8b980-0fd2-4e4d-a9dc-9ea15e482833&u=ht
>> >tps%3A%2F%2Fgit.samba.org%2F%3Fp%3Dsamba.git%3Ba%3Dblob%3Bf%3Dlibrpc%2F
>> >idl%2Fxattr.idl
>> >
>> >   In order to make it possible to use the same filesystem with both servers
>> >   it would be great if the well established way used in Samba would be used
>> >   as well.
>>
>> A thousand times this ! If cifs.ko->ksmbd adds a differnt way of storing the extra meta-data that is
>> incompatible with Samba this would be a disaster for users.
>>
>> Please fix this before proposing any merge.
>You said that samba can handle it even if ksmbd has own extra metadata format. I didn't think it was
>necessary to what you said. If we have to do this, I think it is not too late to work after sending
>ksmbd to linux-next first.

Yes, Samba could add an adaptor to read the ksmbd metadata format,
but then files written by Samba would have metadata unreadable
by ksmbd.

As the Samba metadata format is widely used then I don't see
the purpose of creating a new format for this. It's needless
incompatibility. Please fix this before submitting anywhere.

Re: updated ksmbd (cifsd)

[Stefan Metzmacher]

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

Am 15.12.20 um 03:28 schrieb Namjae Jeon via samba-technical:
>  
>> I just looked briefly, but I'm wondering about a few things:
>>
>> 1. The xattr's to store additional meta data are not compatible with
>>    Samba's way of storing things:
>>     https://git.samba.org/?p=samba.git;a=blob;f=librpc/idl/xattr.idl
>>
>>    In order to make it possible to use the same filesystem with both servers
>>    it would be great if the well established way used in Samba would be used
>>    as well.
> Yes, Maybe... I will check it after sending ksmbd to linux-next.
>  
>> 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://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-fsa/8ada5fbe-db4e-49fd-aef6-20d54b748e40

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

>> 5. Documentation/filesystems/cifsd.rst and fs/cifsd/Kconfig still references https://github.com/cifsd-team/cifsd-tools
>>   instead of https://github.com/cifsd-team/ksmbd-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


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

Re: updated ksmbd (cifsd)

[Sergey Senozhatsky]

On (20/12/15 15:29), Stefan Metzmacher wrote:
> >> 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?

The idea was that user-space needs to have its own user:group
(e.g. CIFSD:CIFSD). And smb.conf and password file should not
be readable by anyone who's not from CIFSD:CIFSD - similar to
how .ssh/config is 0700 on any reasonably configured system.

The massive security problem here is that the server runs in
the kernel. So I don't always see why people want to also run
user-space (which serves RPC calls, and technically can be
tricked to do something that it was not intended to do) under
root - wouldn't this just increases the attack surface?

	-ss

Re: updated ksmbd (cifsd)

[Sergey Senozhatsky]

On (20/12/16 12:24), Sergey Senozhatsky wrote:
> On (20/12/15 15:29), Stefan Metzmacher wrote:
> > >> 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?
> 
> The idea was that user-space needs to have its own user:group
> (e.g. CIFSD:CIFSD). And smb.conf and password file should not
> be readable by anyone who's not from CIFSD:CIFSD - similar to
> how .ssh/config is 0700 on any reasonably configured system.
> 
> The massive security problem here is that the server runs in
> the kernel. So I don't always see why people want to also run
> user-space (which serves RPC calls, and technically can be
> tricked to do something that it was not intended to do) under
> root - wouldn't this just increases the attack surface?

So SMB_SERVER_CHECK_CAP_NET_ADMIN enforces the "user-space must
be a privileged process" policy. Even CAP_NET_ADMIN is too huge,
not to mention that _probably_ this CAP requirement means that
people will just "sudo cifsd". One way or another a malformed
RPC request can do quite a bit of damage to the system, because
user-space runs with the CAPs it doesn't really need.

It would be better to enforce a different policy, IMHO.
Something like:

	groupadd ... CIFSD_GROUP
	useradd -g CIFSD_GID -p CIFSD_PASSWORD CIFSD_LOGIN
	chmod 0700 smb.conf and password db
	chown CIFSD_LOGIN:CIFSD_GROUP smb.conf and password db

And perhaps we need to add some checks to the user-space cifsd:
make sure that smb.conf and password db are 0700 + some more.

	-ss

RE: updated ksmbd (cifsd)

[Namjae Jeon]

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

Re: updated ksmbd (cifsd)

[Sergey Senozhatsky]

On (20/12/16 13:21), Sergey Senozhatsky wrote:
> So SMB_SERVER_CHECK_CAP_NET_ADMIN enforces the "user-space must
> be a privileged process" policy. Even CAP_NET_ADMIN is too huge,
> not to mention that _probably_ this CAP requirement means that
> people will just "sudo cifsd". One way or another a malformed
> RPC request can do quite a bit of damage to the system, because
> user-space runs with the CAPs it doesn't really need.
> 
> It would be better to enforce a different policy, IMHO.
> Something like:
> 
> 	groupadd ... CIFSD_GROUP
> 	useradd -g CIFSD_GID -p CIFSD_PASSWORD CIFSD_LOGIN
> 	chmod 0700 smb.conf and password db
> 	chown CIFSD_LOGIN:CIFSD_GROUP smb.conf and password db

Just a sketch. Completely untested (wasn't even compile tested).
Merely an idea.

This removes the requirement for user-space to run as a privileged
process. Because "let's grant some random user-space daemon
administrative privileges" most likely doesn't improve security
of the system.

So the idea is to provide CIFSD administrator UID and GID during
kcifsd modprobe and then in IPC handlers check if the app issuing
a netlink syscall has the same UID and GID as the CIFSD administrator
or not.

This untested, sorry. Just checking the idea.

---

diff --git a/fs/cifsd/Kconfig b/fs/cifsd/Kconfig
index e12616cf8477..9f221a37223d 100644
--- a/fs/cifsd/Kconfig
+++ b/fs/cifsd/Kconfig
@@ -50,14 +50,6 @@ config SMB_SERVER_SMBDIRECT
 	  SMB Direct allows transferring SMB packets over RDMA. If unsure,
 	  say N.
 
-config SMB_SERVER_CHECK_CAP_NET_ADMIN
-	bool "Enable check network administration capability"
-	depends on SMB_SERVER
-	default n
-
-	help
-	  Prevent unprivileged processes to start the cifsd kernel server.
-
 config SMB_SERVER_KERBEROS5
 	bool "Support for Kerberos 5"
 	depends on SMB_SERVER
diff --git a/fs/cifsd/server.c b/fs/cifsd/server.c
index b9e114f8a5d2..dbbdb3503b0d 100644
--- a/fs/cifsd/server.c
+++ b/fs/cifsd/server.c
@@ -25,6 +25,7 @@
 
 int ksmbd_debug_types;
 
+static int admin_uid = -1, admin_gid = -1;
 struct ksmbd_server_config server_conf;
 
 enum SERVER_CTRL_TYPE {
@@ -335,6 +336,26 @@ static void server_conf_free(void)
 	}
 }
 
+static int server_admin_cred_init(void)
+{
+	if (admin_uid == -1 || admin_gid == -1)
+		return 0;
+
+	server_conf.admin_cred = kmalloc(sizeof(struct admin_cred), GFP_KERNEL);
+	if (!server_conf.admin_cred)
+		return -ENOMEM;
+
+	server_conf.admin_cred.uid = make_kuid(&init_user_ns, admin_uid);
+	server_conf.admin_cred.gid = make_kgid(&init_user_ns, admin_gid);
+	if (!(uid_validserver_conf.admin_cred.uid() &&
+	      gid_valid(server_conf.admin_cred.gid))) {
+		kfree(server_conf.admin_cred);
+		server_conf.admin_cred = NULL;
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int server_conf_init(void)
 {
 	WRITE_ONCE(server_conf.state, SERVER_STATE_STARTING_UP);
@@ -343,10 +364,9 @@ static int server_conf_init(void)
 	server_conf.max_protocol = ksmbd_max_protocol();
 	server_conf.auth_mechs = KSMBD_AUTH_NTLMSSP;
 #ifdef CONFIG_SMB_SERVER_KERBEROS5
-	server_conf.auth_mechs |= KSMBD_AUTH_KRB5 |
-				KSMBD_AUTH_MSKRB5;
+	server_conf.auth_mechs |= KSMBD_AUTH_KRB5 | KSMBD_AUTH_MSKRB5;
 #endif
-	return 0;
+	return server_admin_cred_init();
 }
 
 static void server_ctrl_handle_init(struct server_ctrl_struct *ctrl)
@@ -418,6 +438,21 @@ int server_queue_ctrl_reset_work(void)
 	return __queue_ctrl_work(SERVER_CTRL_TYPE_RESET);
 }
 
+int server_validate_admin_cred(void)
+{
+	if (admin_uid == -1 || admin_gid == -1)
+		return 0;
+
+	/* We couldn't init server admin UID/GID */
+	if (!server_conf.admin_cred)
+		return -EINVAL;
+
+	if (uid_eq(task_uid(current), server_conf.admin_cred.uid) &&
+	    gid_eq(task_gid(current), server_conf.admin_cred.gid))
+		return 0;
+	return -EINVAL;
+}
+
 static ssize_t stats_show(struct class *class,
 			  struct class_attribute *attr,
 			  char *buf)
@@ -614,6 +649,11 @@ static void __exit ksmbd_server_exit(void)
 	ksmbd_release_inode_hash();
 }
 
+module_param(admin_uid, int, 0);
+MODULE_PARM_DESC(admin_uid, "ksmb administrator user id");
+module_param(admin_gid, int, 0);
+MODULE_PARM_DESC(admin_gid, "ksmb administrator group id");
+
 MODULE_AUTHOR("Namjae Jeon <linkinjeon@kernel.org>");
 MODULE_VERSION(KSMBD_VERSION);
 MODULE_DESCRIPTION("Linux kernel CIFS/SMB SERVER");
diff --git a/fs/cifsd/server.h b/fs/cifsd/server.h
index 7b2f6318fcff..ed0249061470 100644
--- a/fs/cifsd/server.h
+++ b/fs/cifsd/server.h
@@ -19,6 +19,11 @@
 
 extern int ksmbd_debugging;
 
+struct admin_cred {
+	kuid_t			uid;
+	kgid_t			gid;
+};
+
 struct ksmbd_server_config {
 	unsigned int		flags;
 	unsigned int		state;
@@ -34,6 +39,7 @@ struct ksmbd_server_config {
 	struct smb_sid		domain_sid;
 	unsigned int		auth_mechs;
 
+	struct admin_cred	*admin_cred;
 	char			*conf[SERVER_CONF_WORK_GROUP + 1];
 };
 
@@ -57,6 +63,7 @@ static inline int ksmbd_server_configurable(void)
 	return READ_ONCE(server_conf.state) < SERVER_STATE_RESETTING;
 }
 
+int server_verify_admin_cred(void);
 int server_queue_ctrl_init_work(void);
 int server_queue_ctrl_reset_work(void);
 #endif /* __SERVER_H__ */
diff --git a/fs/cifsd/transport_ipc.c b/fs/cifsd/transport_ipc.c
index 5f24a1ed5c34..b2a42f6ce6e3 100644
--- a/fs/cifsd/transport_ipc.c
+++ b/fs/cifsd/transport_ipc.c
@@ -345,10 +345,8 @@ static int handle_startup_event(struct sk_buff *skb, struct genl_info *info)
 {
 	int ret = 0;
 
-#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN
-	if (!netlink_capable(skb, CAP_NET_ADMIN))
+	if (server_validate_admin_cred())
 		return -EPERM;
-#endif
 
 	if (!ksmbd_ipc_validate_version(info))
 		return -EINVAL;
@@ -401,10 +399,8 @@ static int handle_generic_event(struct sk_buff *skb, struct genl_info *info)
 	int sz;
 	int type = info->genlhdr->cmd;
 
-#ifdef CONFIG_SMB_SERVER_CHECK_CAP_NET_ADMIN
-	if (!netlink_capable(skb, CAP_NET_ADMIN))
+	if (server_validate_admin_cred())
 		return -EPERM;
-#endif
 
 	if (type >= KSMBD_EVENT_MAX) {
 		WARN_ON(1);