smb1 + android error/crashes.

[Andy2244]

I just updated our latest version and now i build with smb1 included, so older devices and Android can work, yet now i have a report with massive problems?

So can you take a log at the debug/errors, see comments on this PR.

openwrt/packages#11290

[namjaejeon]

Checking, Thanks for your report!

[namjaejeon]

@BrainSlayer Sebastian, Have you ever check smb1 working on big endian target ? dd-wrt seems to have the same issue. And it's hard to verify although I tried to find missing endian conversion on smb1 because I don't have a big endian target, can you help me improve? I applied current endian changes to ksmbd-next.

[luizluca]

I don't have a big endian target

You can use qemu-system-mips, which is big endian. You can even try qemu-system-mipsel (little endian) to test a endianness-only issue. I shared owrt script that can download and run an OpenWrt machine. However, openwrt is currently only building big endian (malta/be) machines, which is enough for your tests.

I'll stop (ab)using OpenWrt PR. It's better to deal directly here. I'm following ksmb(-tools)?-next now, building each 6h.

This is my fresh results (it is getting much better):

It looks like android clients (all of them but vlc) are failing to chdir with "Invalid handler". I can see share directory content but when I enter "dirxyz", it shows an empty dir with a "dirxyz". I can keep entering this "dirxyz" forever.
Maybe client could have checked somehow it failed to chdir, but it should have worked: android-endlessdirs.pcapng.gz

It also fails to read a file: android-fail-to-read.pcapng.gz

vlc does not generate the "Invalid handler" and it browser nicely: vlc-reading-ok.pcapng.gz

From dmesg, I see some error messages (not sure when they happen):

[306310.569355] ksmbd: ksmbd_validate_filename:93: File name validation failed: 0xe
[306353.387696] ksmbd: need to set last modified time before close
[306383.451983] ksmbd: ksmbd_conn_write:214: Failed to send message: -131
[306383.678395] ksmbd: need to set last modified time before close
[306405.444573] ksmbd: need to set last modified time before close
[306469.372836] ksmbd: need to set last modified time before close

[namjaejeon]

Note: Hyunchul lee will check smb1 endian issue on next week. Please wait for him to fix it.

[BrainSlayer]

i need todo more excessive tests on my big endian systems. with the current revision i dont see much problems. the last issue i had was the cifs client message Bad SMB: : dump of 48 bytes of data at 0x87366740  40000000 424d53fe 00020040 00000000 . . . @   S M B @ . . . . . . .  00020010 00000001 00000000 00000009 . . . . . . . . . . . . . . . .  00000000 00000644 00000000 00000050 . . . . D . . . . . . . P . . . the problem is just that the server was little endian x64 here and it was using smb 3 i will setup some test server to check if smb1 is working with mips be Am 17.02.2020 um 22:20 schrieb Namjae Jeon:

…

@BrainSlayer <https://github.com/BrainSlayer> Sebastian, Have you ever check smb1 working on big endian target ? dd-wrt seems to have the same issue. And it's hard to verify although I tried to find missing endian conversion on smb1 because I don't have a big endian target, can you help me improve? I applied current endian changes to ksmbd-next. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#334>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AB2WNE4C34IA5TMLSUPUIKDRDL5STANCNFSM4KUOIITQ>.

[hclee]

I will check this problem in the next week.

[hclee]

Hello @luizluca ,

I analyzed your dump files.

Cause of the "Invalid handle" failure is that Android clients try to close a directory that is already closed. A server closes the directory automatically if the end of the directory is reached. So vlc does not try to close the directory.

but when I enter "dirxyz", it shows an empty dir with a "dirxyz". I can keep entering this "dirxyz" forever.
Maybe client could have checked somehow it failed to chdir, but it should have worked

Sorry, I cannot understand what problem is about "dirxyz" exaclty. Could you explain it by example?

It also fails to read a file: android-fail-to-read.pcapng.gz

There are no read requests issued by a client in the dump file. Everything is okay.

[luizluca]

Sorry, I cannot understand what problem is about "dirxyz" exaclty. Could you explain it by example?

If I have this directory structure:

• folder1
  • folder1.1
  • file1
  • file2
• folder2

It lists at share root (as expected):

• folder1
• folder2

But when I enter folder1, it shows again and only folder1 as a subfolder of folder1, not folder1.1 or file[12]. My client acts as if it was:

//Server/share/folder1/folder1/folder1/folder1/...

It works with samba3.

[namjaejeon]

@hclee regarding to invalid handle error when closing directory, we need to try to reproduce it again on ksmbd-next. I fixed a lot of endian issues on ksmbd-next. maybe, we should try to reproduce it after installing apps in smartphone.

[namjaejeon]

It looks like android clients (all of them but vlc) are failing to chdir with "Invalid handler". I can see share directory content but when I enter "dirxyz", it shows an empty dir with a "dirxyz". I can keep entering this "dirxyz" forever.
Maybe client could have checked somehow it failed to chdir, but it should have worked: android-endlessdirs.pcapng.gz

@luizluca Is this issue reproducible with ksmbd-next ? I am wondering if your test result was reported on old ksmbd-next. I fixed a lot of conversion issue on ksmbd-next. the last patch of smb1 conversion is this one (ad17779). Could please check your test image contained this change ?

[hclee]

@luizluca Could you capture SMB packets sent between a little endian ksmbd and an android client. I cannot test it because of vacation. the search pattern for looking up directory entries in client's SMB request is strange.

[luizluca]

@luizluca Is this issue reproducible with ksmbd-next ?

I just tested it with a built from 20 minutes ago. It's just the same.

I was not able to take a good look at it but only ksmbd shows "." and "..". So, it askes for "." content. After that, instead of asking for "dir/*" (as it does with samba) , it askes for "dir/". This will give me the endless subdir behavior.

I'll try to capture more data still today and also do a little endian version test.

[luizluca]

I'm doing a side-by-side debug and found some interesting things:

• The last "Session Setup AndX Response" (4th in the 4-step authentication) have bytecount==0 in ksmbd while samba informs server SO, samba version and primary domain. It might be optional.
• ksmbd uses id==0 (fid, session key, tree id, referent id, search id) when samba uses zero only for an absent field. For example, in "Tree Connect AndX Response", where ksmbd uses "tree id"==0, samba uses "tree id"==2. Wireshark gets confused by "tree id"==0 with data mentioning it in requests that does not really have "tree id" set. Another example is "session key", which does not looks like something that should be zero as ksmbd does with "Negotiate Protocol Response" (samba has a random(?) 16-bit value in a 32-bit field). Further, ksmbd uses a sequential value for session while samba uses something harder to guess. Should it be random?
• In FIND_FIRST2 (0x0001), Search Pattern: "*", FIND_FIRST2 Data says "." has filename size of 4 while samba says it is 2 (UTF-16?). If the client uses the filename size in anyway, a size of 4 would break. ".." is 6 bytes for ksmbd while samba says it is 4 bytes. ksmbd is always adding 2 bytes to every filename size.
• It seems the client correctly sends find_close2_request (with treeid==1, userid==2 and searchid==0). For samba, all ids are non-zero. For ksmbd, search_id is zero. It looks like "search id" from wireshark is indeed "smb_com_findclose_req.FileID" in ksmbd. I tried to look the code but as far as I went, nothing prevents it from being zero. However, somewhere it is returning an error.
• FIND_FIRST2 data entries seem to use a non-initialized value for created time (it looks random). If it is uninitialized memory, it could leak sensitive data. Samba made it equals to last access. It would be wise to set it to something similar to other fields to avoid discrepancy. I would not recommend epoch==0.
• When I click on a directory, with samba, it runs FIND_FIRST2 with pattern \mydir*. WIth ksmbd, it does only \mydir. This is what makes the client "see" a subdir inside each directory with the same same as the parent directory.

My guess is that the extra filename size adds an extra \0 at the end of the filename. If the client is not cautious, it might simply do:

FIND_FIRST2_request.pattern = strncat(FIND_FIRST2_response.data[0].filename[FIND_FIRST2_response.data[0].filename_size],"\\*",2);

And the pattern that should be '\mydir\' will be '\mydir\0\' or, when considering a null terminate string, only ''\mydir' (which is what I see the client asking). With that in mind, I checked the bytestream:

• ksmbd: 5c 00 62 00 69 00 6e 00 00 00 5c 00 2a 00 00 00 : "\bin\0\*\0"
• samba: 5c 00 62 00 69 00 6e 00 5c 00 2a 00 00 00: "\bin\*\0"

This is quite certain that the issue is simply filename size which is off by one in ksmbd, manifested with a poor implemented client (it looks like it is using JCIFS).

There is still the "Invalid handler" when the client tries to close a find result. It also happened with little endian (armvirt). Maybe something in ksmbd does not expect search_id/file_id == 0.

Because of the off-by-one bug in filename, I could no test it further. However, I could not spot a single step where little/big endian systems are different.

I'm now building each 6 hours ksmbd for openwrt 19.07.1 for both ath79/generic (mips big endian) and armvirt/32 (ARM little endian). I'm not testing them everyday but it might be easier to test with a pre-built package.

[hclee]

@luizluca

Thank you for the detailed explanation ;)

My guess is that the extra filename size adds an extra \0 at the end of the filename. If the client is not cautious, it might simply do:

I also found this problem. As you said, the cause of keeping entering a directory forever is '\0' in the search pattern. What I am confused is why this problem is not reproducible in little endian.

We have the bug to count the length of a filename including '\0' character. I will send the patch to fix it.

There is still the "Invalid handler" when the client tries to close a find result. It also happened with little endian (armvirt). Maybe something in ksmbd does not expect search_id/file_id == 0.

I think this issue is not related with search id == 0. If there is nothing left in a directory, ksmbd closes the handle of the directory (search id) automatically, and let a client know the end of a search. vlc and cifs filesystem does not try to close the search id in this case. But the android apps tries.

Does not this issue come up for Samba?

[hclee]

@luizluca

There is still the "Invalid handler" when the client tries to close a find result. It also happened with little endian (armvirt). Maybe something in ksmbd does not expect search_id/file_id == 0.

I think this issue is not related with search id == 0. If there is nothing left in a directory, ksmbd closes the handle of the directory (search id) automatically, and let a client know the end of a search. vlc and cifs filesystem does not try to close the search id in this case. But the android apps tries.

Does not this issue come up for Samba?

We have the bug. we must close the directory if the "Close on EOS" flag is set. I will send the patch to fix it.

Thank you!

[namjaejeon]

@luizluca Push Hyunchul's patches into ksmbd-next now.

[luizluca]

@namjaejeon and @hclee , finally I can use all my Android apps! Thank you.

I would still take a look on the random creation time field.

[namjaejeon]

@luizluca It was possible with your detailed description. Thank you very much!
@hclee Thanks for your work!

[namjaejeon]

I would still take a look on the random creation time field.
@luizluca Okay. I am glad if you fix it.

[namjaejeon]

the last issue i had was the cifs client message
Bad SMB: : dump of 48 bytes of data at 0x87366740
40000000 424d53fe 00020040 00000000 . . . @ S M B @ . . . . . . .
00020010 00000001 00000000 00000009 . . . . . . . . . . . . . . . .
00000000 00000644 00000000 00000050 . . . . D . . . . . . . P . . .
the problem is just that the server was little endian x64 here and it
was using smb 3

@BrainSlayer Very stranged. Could you please fully turn on debug message of cifs client ?
https://wiki.samba.org/index.php/LinuxCIFS_troubleshooting

[namjaejeon]

I would still take a look on the random creation time field.

@luizluca Any news ? Could you explain more about symptom ? I fixed the issue that creation time is not set for dot and dotdot file on #ksmbd-next. not sure it is the same issue that you are facing.

[luizluca]

@namjaejeon , I just tested it (2020-03-17) and I do not see that random creation time anymore. Thanks.

I guess this fixes all issues I saw with android clients. @Andy2244 , @namjaejeon , this issue is ready to close.

[namjaejeon]

@luizluca Okay! Thanks for your feedback!
@Andy2244 I will push changes from #ksmbd-next to #master tonight. You can take next version tomorrow or when you have free time for release. Thanks!

[namjaejeon]

@Andy2244 Release ksmbd 3.1.6 version and ksmbd-tools 3.2.3 version now.

[Andy2244]

ok thanks for the info, will try make a new release this week.

[luizluca]

ok thanks for the info, will try make a new release this week.

@Andy2244 , please consider that whenever you touch a kmod package, openwrt buildbot remove older kmods while the new one is compiled only for openwrt-19.07-snapshot only. If its kernel version differ from the release one, you'll have your kmod in a broken state until next release. OpenWrt does have a spot to place multiple kmod versions for each kernel release but it looks like it's not used for openwrt-packages out off the kernel modules, only for kmods from the kernel source.

I sent an email to ML asking about compiling kmods for each stable release but I got no answer.

[Neustradamus]

I am lost, it is cifsd or ksmbd?

[namjaejeon]

cifsd is directory and project name. ksmbd is thread(task) name. cifsd == ksmbd.
https://git.samba.org/?p=sfrench/cifsd.git;a=commit;h=57a9c34a861a6defd7c7b06ccbcc6ec7a28f38ac

[Neustradamus]

@namjaejeon: You have not changed the name of the project and the soft recently?
Like you know, I follow the project since a long time: cifsd -> smbd -> ksmbd -> cifsd.

And any news to move the repositories:

• cifsd-team/cifsd -> cifsd-team/cifsd-old
• namjaejeon/cifsd -> cifsd-team/cifsd
• ...

The goal is to have the perfect code in cifsd-team.

For example, Microsoft has not main projects in user accounts.

[namjaejeon]

@Andy2244 Can I close this ISSUE if release is done ?

[Andy2244]

sure