From: Paul Gortmaker paul.gortmaker@windriver.com
This is a bit long, but I've never touched this code and all I can do is compile test it. So the below basically represents a capture of my thought process in fixing this for the v5.15.y-stable branch.
I am hoping the folks who normally work with this code can double check that I didn't get off-track somewhere...
CVE-2023-38431 points at commit 368ba06881c3 ("ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop") as the fix:
https://nvd.nist.gov/vuln/detail/CVE-2023-38431
For convenience, here is a link to the fix:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/fs...
It was added in v6.4
git describe --contains 368ba06881c3 v6.4-rc6~2^2~1
...and backported to several stable releases. But just not v5.15.
Why not v5.15? If we look at the code the fix patches with "git blame" we get commit 0626e6641f6b4 ("cifsd: add server handler for central processing and tranport layers")
$git describe --contains 0626e6641f6b4 v5.15-rc1~183^2~94
So that would have been the commit the "Fixes:" line would have pointed at if it had one.
Applying the fix to v5.15 reveals two problems. The 1st is a trivial file rename (fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15) and then the commit *applies*. The 2nd problem is only revealed at compile time...
The compile fails because the v5.15 baseline does not have smb2_get_msg(). Where does that come from?
commit cb4517201b8acdb5fd5314494aaf86c267f22345 Author: Namjae Jeon linkinjeon@kernel.org Date: Wed Nov 3 08:08:44 2021 +0900
ksmbd: remove smb2_buf_length in smb2_hdr
git describe --contains cb4517201b8a v5.16-rc1~21^2~6
So now we see why v5.15 didn't get a linux-stable backport by default. In cb4517201b8a we see:
+static inline void *smb2_get_msg(void *buf) +{ + return buf + 4; +}
However we can't just take that context free of the rest of the commit, and then glue it into v5.15. The whole reason the function exists is because a length field of 4 was removed from the front of a struct. If we look at the typical changes the struct change caused, we see:
- struct smb2_hdr *rcv_hdr2 = work->request_buf; + struct smb2_hdr *rcv_hdr2 = smb2_get_msg(work->request_buf);
If we manually inline that, we obviously get:
- struct smb2_hdr *rcv_hdr2 = work->request_buf; + struct smb2_hdr *rcv_hdr2 = work->request_buf + 4;
Now consider the lines added in the fix which is post struct reduction:
+#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr) + 4)
+ if (((struct smb2_hdr *)smb2_get_msg(conn->request_buf))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) + break; + }
...and if we inline/expand everything, we get:
+ if (((struct smb2_hdr *)(conn->request_buf + 4))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < (sizeof(struct smb2_hdr) + 4)) + break; + }
And so, by extension the v5.15 code, which is *pre* struct reduction, would simply not have the "+4" and hence be:
+ if (((struct smb2_hdr *)(conn->request_buf))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < (sizeof(struct smb2_hdr))) + break; + }
If we then put the macro back (without the 4), the v5.15 version would be:
+#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr))
+ if (((struct smb2_hdr *)(conn->request_buf))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) + break; + }
And so that is what I convinced myself is right to put in the backport.
If you read/reviewed this far - thanks! Paul.
---
Namjae Jeon (1): ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop
fs/ksmbd/connection.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)