From: Paul Gortmaker <paul.gortmaker@xxxxxxxxxxxxx>
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/smb/server?id=368ba06881c3
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@xxxxxxxxxx>
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(+)
--
2.40.0
