Work's aux_payload cause error if the read command rap in middle of chained-compounded response

[MiaoLiHua]

@namjaejeon

I use MacOS as client connect to ksmbd. The macOS occasionally send compounded request includes: smb2_open, smb2_read(read a xattr) and the smb2_close. For the read command isn't the last one, the work's response_buf has hole while set a new next_smb2_rsp_hdr_off in the function "init_chained_smb2_rsp". And, smb3_encrypt_resp and ksmbd_conn_write also not process the last smb2_close's response correctly.

I suggest a casual solution, insert following codes into function init_chained_smb2_rsp line 383: if (work->aux_payload_sz) { //assert(work->resp_hdr_sz-4+work->aux_payload_sz==get_rfc1002_len(work->response_buf)); //transfer the previously last command read aux_payload to work->response_buf memcpy(work->response_buf+work->resp_hdr_sz, work->aux_payload_buf, work->aux_payload_sz);

kvfree(work->aux_payload_buf); work->aux_payload_buf = NULL; work->aux_payload_sz = 0; work->resp_hdr_sz = 0; }

That is right?

[namjaejeon]

Thanks for your report:) This is workaround and It will cause side effect. Okay. I understood what problem is. I will check it.

[namjaejeon]

memcpy cause performance drop and memory overrun if ->aux_payload_buf size is bigger than ->response_buf. Anyway, Your understanding of ksmbd code and smb protocol is amazing :)

[MiaoLiHua]

memcpy cause performance drop and memory overrun if ->aux_payload_buf size is bigger than ->response_buf.

You are right, although "is_chained_smb2_message" line 450 already check the response_buf's size, anyway the simple workaround not well enough. Change work's aux_payload to a list of aux_payloads, and every aux_payload record itself insert point offset relative response_buf begin may be a thorough solution. But, that will cause a lot of modifications.

By the way, I think smb3_encrypt_resp function line 8733 could strike out possibly, once ksmbd_conn_write precisely set every iv's base.

[namjaejeon]

Change work's aux_payload to a list of aux_payloads, and every aux_payload record itself insert point offset relative response_buf begin may be a thorough solution. But, that will cause a lot of modifications.

Yes. good. Do you have the time to work this ?

By the way, I think smb3_encrypt_resp function line 8733 could strike out possibly, once ksmbd_conn_write precisely set every iv's base.

Hm... Good point, And we should encrypt smb header + payloads except transform header before sending it. I agreed that It should be changed for the case(smb2_open + smb2_read + smb2_cloase).

[MiaoLiHua]

Change work's aux_payload to a list of aux_payloads, and every aux_payload record itself insert point offset relative response_buf begin may be a thorough solution. But, that will cause a lot of modifications.

Yes. good. Do you have the time to work this ?

I am coding at Mac, so I have not a ready ubuntu debug environment, specially for kernel module I am unfamiliar. I absolutely depend on log to dig ksmbd now, so modify much codes will be very inefficient. If I do this improvement, may be take a long time. That is OK?

Another idea, let transform header reside at begin of response_buf throughout, because nowaday most responses needs it. It replace the independent tr_buf.

[namjaejeon]

If I do this improvement, may be take a long time. That is OK?

Up to you. If you can do it, I will wait:)

Another idea, let transform header reside at begin of response_buf throughout, because nowaday most responses needs it. It replace the independent tr_buf.

transform header is used for only encryption mode. I haven't understood exactly how you're going to do it yet, but after implementing it, let's review it together!

[MiaoLiHua]

If I do this improvement, may be take a long time. That is OK?

Up to you. If you can do it, I will wait:)

Let me try it. Would you like tell me what debug tools you are using for the kernel module developing? Just raw GDB?

[namjaejeon]

Let me try it. Would you like tell me what debug tools you are using for the kernel module developing? Just raw GDB?

I don't use any debugger for kernel module development. I prefer using printk(pr_err()) like Linus. Other kernel hacker also seems to use it. If you have any question during working it, Let me know it.

[MiaoLiHua]

Oh, fine. Thank you, if I occurred some difficult while practicing, I would ask your help:)

[MiaoLiHua]

@namjaejeon

I have create a local new branch "aux_payload" and I made some codings. But I can not git push the branch to GitHub by Sourcetree git client. It report following message:

Pushing to https://github.com/namjaejeon/ksmbd.git remote: Invalid username or password. fatal: Authentication failed for 'https://github.com/namjaejeon/ksmbd.git/' Pushing to https://github.com/namjaejeon/ksmbd.git

That needs you do something to allow I push? Or, I compress modified files to zip and submit to you?

[namjaejeon]

@lihua-theone Ah.. once, backup your patch.

First of all, You should fork https://github.com/namjaejeon/ksmbd. and you can see forked git tree(https://github.com/lihua-theone/ksmbd) in your github account. And create new branch ""aux_payload" on https://github.com/lihua-theone/ksmbd and push it with your account and password of github. And send a PULL request to https://github.com/namjaejeon/ksmbd master branch from https://github.com/lihua-theone/ksmbd. Let me know if you have any question more.

[MiaoLiHua]

@namjaejeon

Thanks:) I have forked and push my codes and sent a PULL request. You could pull and review it.

[MiaoLiHua]

@namjaejeon

Here has the following message: Travis CI / Travis CI - Pull Request The build failed, just like the previous build.

What needs I do for it?

[namjaejeon]

The build failed, just like the previous build.

Yes, I need to move test setup from Travis-CI to github action. So we can ignore Travis-CI's result now. I will do manually test it after reviewing the patch.

[MiaoLiHua]

Okey, thanks. Let me know if here have any uncertains.

[namjaejeon]

And.. You need to add prefix(ksmbd:) in patch subject, i.e. "ksmbd: Change the work's aux_payload_buf to aux_payload_list"

And need to add patch description also. Please check other commits(https://github.com/namjaejeon/ksmbd/commit/dbd01c3a5543fb7f0dabca7a2f05d00ae21d80f6)

[MiaoLiHua]

I have almost finished all you mentioned. But, "patch description" just is the commit message? I do last commit with a similar description. That is all right?:)

And need to add patch description also. Please check other commits(https://github.com/namjaejeon/ksmbd/commit/dbd01c3a5543fb7f0dabca7a2f05d00ae21d80f6)

[namjaejeon]

@MiaoLiHua Sorry for long delay. Could you please check this branch to know if your problem is improved or not ?

git clone https://github.com/namjaejeon/ksmbd/tree/comp_readv3

Thanks.

[namjaejeon]

@MiaoLiHua Ping?

[namjaejeon]

Fixed. please reopen if it is not fixed yet.