That Didn't Take Long: KSMBD In-Kernel File Server Already Needs Important Security Fix

[sverris]

"That Didn't Take Long" - which is a good thing. Fixing young code is always better than fixing old code. For obvious reasons.

[ssokolow]

"That Didn't Take Long" - which is a good thing. Fixing young code is always better than fixing old code. For obvious reasons.

Yes, but code that doesn't need to be fixed is better than code that's fixed quickly.

[mdedetrich]

Rust would have helped with this part:



The failing to normalize paths before enforcing .. is just an embarrassing beginner's mistake. (I knew to make sure I was doing that for the IRC fserve script I never finished back in my late high school and early university years!)

Of course, in my case, it was more or less because I'd realized early on that the best way to handle security is to make it someone else's fault if the relevant code proves to be broken.

Indeed, the buffer overflow would have automatically been checked by Rust's borrow checker at compile time (so the code wouldn't have even shipped). The error paths is a business logic error, honestly there should have been a test for this.

[Space Heater]

Imagine if Microsoft would have put SMB/CIFS in the Windows kernel, we would all laugh and ridicule them!

I'm not sure if this is a serious comment or not, but the Windows smb client and server do run in the kernel.
There was even a presentation at a samba conference about this. If you want to look into this your self, boot up a Windows system and look in Windows\system32\drivers for mrxsmb.sys, mrxsmb20.sys,srv.sys, srvnet.sys, and srv2.sys.

I'll also add that macOS has a smb in-kernel driver (e.g. System/Library/Extensions/smbfs.kext), as does Solaris.

[mdedetrich]

I'm not sure if this is a serious comment or not, but the Windows smb client and server do run in the kernel.
There was even a presentation at a samba conference about this. If you want to look into this your self, boot up a Windows system and look in Windows\system32\drivers for mrxsmb.sys, mrxsmb20.sys,srv.sys, srvnet.sys, and srv2.sys.

I'll also add that macOS has a smb in-kernel driver (e.g. System/Library/Extensions/smbfs.kext), as does Solaris.

Its also inside the FreeBSD kernel

[Awesomeness]

still quite embarrassing "beginners" bug :/

Most embarrassing for the professionals who reviewed the code. That's what code review is for.

[rene]

if just some news site would not praise all obviously bad ideas in the name of performance ;-) ¯\_(ツ)_/¯

[rene]

I still think smb belongs in userland (same with nfs). AND all reasons why it is better in kernel should be solved so that anyone can use them from userland.

its also much easier to update userspace than kernel

^- THIS. ;-) obviously.

[arQon]

If you want performance you don't really have a choice

No, you really do. We've DONE this already, getting samba to sustain GbE line rate on incredibly shitty hardware (as in dual-core 600MHz ARM with no RAM levels of "shitty"). Once you have zerocopy, there's almost nothing that actually *needs* to be in kernelspace because you just aren't making enough roundtrips for it to matter. Electing to massively increase the attack surface of the kernel for the sake of wringing the last few trivial drops of +perf out of something like this is simply a bad decision.

It's not like Linus et al are idiots, and they're the ones who approved this, but IMO that doesn't make it any less of a poor choice. I can understand being motivated to do it by history (e.g. "NFS is in the kernel already" etc), despite that being from a time where there were far fewer attacks taking place and the kernel was 1/1000th of its current size; or to win a benchmark against Windows Server etc, but it's still not the choice I'd have made: not least because eventually someone is going to argue that SMB4 "needs to" be in the kernel too, since NFS and SMB3 are, and SMB4 is a @#$%ing nightmare...

Still, at least THIS one was caught early.

[mdedetrich]

No, you really do. We've DONE this already, getting samba to sustain GbE line rate on incredibly shitty hardware (as in dual-core 600MHz ARM with no RAM levels of "shitty"). Once you have zerocopy, there's almost nothing that actually *needs* to be in kernelspace because you just aren't making enough roundtrips for it to matter. Electing to massively increase the attack surface of the kernel for the sake of wringing the last few trivial drops of +perf out of something like this is simply a bad decision.

It's not like Linus et al are idiots, and they're the ones who approved this, but IMO that doesn't make it any less of a poor choice. I can understand being motivated to do it by history (e.g. "NFS is in the kernel already" etc), despite that being from a time where there were far fewer attacks taking place and the kernel was 1/1000th of its current size; or to win a benchmark against Windows Server etc, but it's still not the choice I'd have made: not least because eventually someone is going to argue that SMB4 "needs to" be in the kernel too, since NFS and SMB3 are, and SMB4 is a @#$%ing nightmare...

Still, at least THIS one was caught early.

You should probably have a read of the proposal because creating a fast SMB implementation is a lot more than just "zero-copy" https://lore.kernel.org/lkml/[email protected]/