public inbox for stable@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431
@ 2023-12-12 18:47 paul.gortmaker
  2023-12-12 18:47 ` [PATCH 1/1] ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop paul.gortmaker
                   ` (2 more replies)
  0 siblings, 3 replies; 22+ messages in thread
From: paul.gortmaker @ 2023-12-12 18:47 UTC (permalink / raw)
  To: Namjae Jeon, Steve French; +Cc: stable

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/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@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(+)

-- 
2.40.0


^ permalink raw reply	[flat|nested] 22+ messages in thread
end of thread, other threads:[~2023-12-18 11:38 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-12 18:47 [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431 paul.gortmaker
2023-12-12 18:47 ` [PATCH 1/1] ksmbd: check the validation of pdu_size in ksmbd_conn_handler_loop paul.gortmaker
2023-12-13  4:59   ` Namjae Jeon
2023-12-18 10:38   ` Greg KH
2023-12-18 11:28     ` Namjae Jeon
2023-12-18 11:38       ` Greg KH
2023-12-12 20:04 ` [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431 Greg KH
2023-12-12 20:13   ` [EXTERNAL] " Steven French
2023-12-12 20:52     ` Paul Gortmaker
2023-12-12 21:15       ` Steven French
2023-12-13 14:36     ` Greg KH
2023-12-13 23:31       ` Namjae Jeon
2023-12-14  8:05         ` Greg KH
2023-12-14 11:33           ` Namjae Jeon
2023-12-14 11:58             ` Greg KH
2023-12-14 13:58               ` Namjae Jeon
2023-12-12 20:45   ` Paul Gortmaker
2023-12-13 14:34     ` Greg KH
2023-12-14  3:28       ` Paul Gortmaker
2023-12-14  8:20         ` Greg KH
2023-12-15  4:18           ` Paul Gortmaker
2023-12-13  5:13 ` Namjae Jeon

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox