2023-12-13 3:47 GMT+09:00, paul.gortmaker@xxxxxxxxxxxxx
<paul.gortmaker@xxxxxxxxxxxxx>:
> 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.
>
Hi Paul,
> If you read/reviewed this far - thanks!
Your backport patch looks good :), I have checked it work fine.
Thanks for your work!
> 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
>
>
