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(+)
From: Namjae Jeon linkinjeon@kernel.org
commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0 upstream.
The length field of netbios header must be greater than the SMB header sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet.
If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. In the function `get_smb2_cmd_val` ksmbd will read cmd from `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN detector to print the following error message:
[ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 ... [ 7.207125] <TASK> [ 7.209191] get_smb2_cmd_val+0x45/0x60 [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 [ 7.209712] ksmbd_server_process_request+0x72/0x160 [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 [ 7.212280] kthread+0x160/0x190 [ 7.212762] ret_from_fork+0x1f/0x30 [ 7.212981] </TASK>
Cc: stable@vger.kernel.org Reported-by: Chih-Yen Chang cc85nod@gmail.com Signed-off-by: Namjae Jeon linkinjeon@kernel.org Signed-off-by: Steve French stfrench@microsoft.com [PG: fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15. Also no smb2_get_msg() as no +4 from cb4517201b8a in v5.15 baseline.] Signed-off-by: Paul Gortmaker paul.gortmaker@windriver.com --- fs/ksmbd/connection.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/fs/ksmbd/connection.c b/fs/ksmbd/connection.c index cab274b77727..7f6fdafa240d 100644 --- a/fs/ksmbd/connection.c +++ b/fs/ksmbd/connection.c @@ -263,6 +263,9 @@ bool ksmbd_conn_alive(struct ksmbd_conn *conn) return true; }
+#define SMB1_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb_hdr)) +#define SMB2_MIN_SUPPORTED_HEADER_SIZE (sizeof(struct smb2_hdr)) + /** * ksmbd_conn_handler_loop() - session thread to listen on new smb requests * @p: connection instance @@ -319,6 +322,9 @@ int ksmbd_conn_handler_loop(void *p) if (pdu_size > MAX_STREAM_PROT_LEN) break;
+ if (pdu_size < SMB1_MIN_SUPPORTED_HEADER_SIZE) + break; + /* 4 for rfc1002 length field */ /* 1 for implied bcc[0] */ size = pdu_size + 4 + 1; @@ -346,6 +352,12 @@ int ksmbd_conn_handler_loop(void *p) continue; }
+ if (((struct smb2_hdr *)(conn->request_buf))->ProtocolId == + SMB2_PROTO_NUMBER) { + if (pdu_size < SMB2_MIN_SUPPORTED_HEADER_SIZE) + break; + } + if (!default_conn_ops.process_fn) { pr_err("No connection request callback\n"); break;
2023-12-13 3:47 GMT+09:00, paul.gortmaker@windriver.com paul.gortmaker@windriver.com:
From: Namjae Jeon linkinjeon@kernel.org
commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0 upstream.
The length field of netbios header must be greater than the SMB header sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet.
If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. In the function `get_smb2_cmd_val` ksmbd will read cmd from `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN detector to print the following error message:
[ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 ... [ 7.207125] <TASK> [ 7.209191] get_smb2_cmd_val+0x45/0x60 [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 [ 7.209712] ksmbd_server_process_request+0x72/0x160 [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 [ 7.212280] kthread+0x160/0x190 [ 7.212762] ret_from_fork+0x1f/0x30 [ 7.212981] </TASK>
Cc: stable@vger.kernel.org Reported-by: Chih-Yen Chang cc85nod@gmail.com Signed-off-by: Namjae Jeon linkinjeon@kernel.org Signed-off-by: Steve French stfrench@microsoft.com [PG: fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15. Also no smb2_get_msg() as no +4 from cb4517201b8a in v5.15 baseline.] Signed-off-by: Paul Gortmaker paul.gortmaker@windriver.com
Looks good to me:) Thanks for your work!
On Tue, Dec 12, 2023 at 01:47:45PM -0500, paul.gortmaker@windriver.com wrote:
From: Namjae Jeon linkinjeon@kernel.org
commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0 upstream.
The length field of netbios header must be greater than the SMB header sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet.
If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. In the function `get_smb2_cmd_val` ksmbd will read cmd from `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN detector to print the following error message:
[ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 ... [ 7.207125] <TASK> [ 7.209191] get_smb2_cmd_val+0x45/0x60 [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 [ 7.209712] ksmbd_server_process_request+0x72/0x160 [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 [ 7.212280] kthread+0x160/0x190 [ 7.212762] ret_from_fork+0x1f/0x30 [ 7.212981] </TASK>
Cc: stable@vger.kernel.org Reported-by: Chih-Yen Chang cc85nod@gmail.com Signed-off-by: Namjae Jeon linkinjeon@kernel.org Signed-off-by: Steve French stfrench@microsoft.com [PG: fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15. Also no smb2_get_msg() as no +4 from cb4517201b8a in v5.15 baseline.] Signed-off-by: Paul Gortmaker paul.gortmaker@windriver.com
fs/ksmbd/connection.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Now queued up, thanks.
greg k-h
2023-12-18 19:38 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 01:47:45PM -0500, paul.gortmaker@windriver.com wrote:
From: Namjae Jeon linkinjeon@kernel.org
commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0 upstream.
The length field of netbios header must be greater than the SMB header sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet.
If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. In the function `get_smb2_cmd_val` ksmbd will read cmd from `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN detector to print the following error message:
[ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 ... [ 7.207125] <TASK> [ 7.209191] get_smb2_cmd_val+0x45/0x60 [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 [ 7.209712] ksmbd_server_process_request+0x72/0x160 [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 [ 7.212280] kthread+0x160/0x190 [ 7.212762] ret_from_fork+0x1f/0x30 [ 7.212981] </TASK>
Cc: stable@vger.kernel.org Reported-by: Chih-Yen Chang cc85nod@gmail.com Signed-off-by: Namjae Jeon linkinjeon@kernel.org Signed-off-by: Steve French stfrench@microsoft.com [PG: fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15. Also no smb2_get_msg() as no +4 from cb4517201b8a in v5.15 baseline.] Signed-off-by: Paul Gortmaker paul.gortmaker@windriver.com
fs/ksmbd/connection.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Now queued up, thanks.
Could you please remove this patch in your queue ? This patches in my ksmbd backport queue and Since I have backported all patches(between 5.16 ~ 6.7-rc1), I included original patch that do not need to be changed unlike this patch.
https://github.com/namjaejeon/stable-linux-5.15-ksmbd
I am testing them before sending it. I plan to send all patches within this week.
Thanks.
greg k-h
On Mon, Dec 18, 2023 at 08:28:17PM +0900, Namjae Jeon wrote:
2023-12-18 19:38 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 01:47:45PM -0500, paul.gortmaker@windriver.com wrote:
From: Namjae Jeon linkinjeon@kernel.org
commit 368ba06881c395f1c9a7ba22203cf8d78b4addc0 upstream.
The length field of netbios header must be greater than the SMB header sizes(smb1 or smb2 header), otherwise the packet is an invalid SMB packet.
If `pdu_size` is 0, ksmbd allocates a 4 bytes chunk to `conn->request_buf`. In the function `get_smb2_cmd_val` ksmbd will read cmd from `rcv_hdr->Command`, which is `conn->request_buf + 12`, causing the KASAN detector to print the following error message:
[ 7.205018] BUG: KASAN: slab-out-of-bounds in get_smb2_cmd_val+0x45/0x60 [ 7.205423] Read of size 2 at addr ffff8880062d8b50 by task ksmbd:42632/248 ... [ 7.207125] <TASK> [ 7.209191] get_smb2_cmd_val+0x45/0x60 [ 7.209426] ksmbd_conn_enqueue_request+0x3a/0x100 [ 7.209712] ksmbd_server_process_request+0x72/0x160 [ 7.210295] ksmbd_conn_handler_loop+0x30c/0x550 [ 7.212280] kthread+0x160/0x190 [ 7.212762] ret_from_fork+0x1f/0x30 [ 7.212981] </TASK>
Cc: stable@vger.kernel.org Reported-by: Chih-Yen Chang cc85nod@gmail.com Signed-off-by: Namjae Jeon linkinjeon@kernel.org Signed-off-by: Steve French stfrench@microsoft.com [PG: fs/smb/server/connection.c --> fs/ksmbd/connection.c for v5.15. Also no smb2_get_msg() as no +4 from cb4517201b8a in v5.15 baseline.] Signed-off-by: Paul Gortmaker paul.gortmaker@windriver.com
fs/ksmbd/connection.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
Now queued up, thanks.
Could you please remove this patch in your queue ? This patches in my ksmbd backport queue and Since I have backported all patches(between 5.16 ~ 6.7-rc1), I included original patch that do not need to be changed unlike this patch.
https://github.com/namjaejeon/stable-linux-5.15-ksmbd
I am testing them before sending it. I plan to send all patches within this week.
Sure, will drop it now, thanks.
greg k-h
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport this, just the presence of a random CVE identifier? If that's all it takes to get companies to actually do backports, maybe I should go allocate more of them :)
thanks,
greg k-h
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
There are arguments to be made for larger backports when test infrastructure is good and lots of good functional tests (due to risk of subtle dependencies when cherrypicking 1 patch out of 5 to backport). In general, Namjae has access to excellent functional/regression suites for SMB server (not just from Samba "smbtorture") so it is theoretically possible to do larger "very safe" backports for ksmbd (or at least make these available as an alternative for users who hit serious roadblocks on older kernels), if the test automation were well trusted. Is there a precedent for this?
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Tuesday, December 12, 2023 2:05 PM To: paul.gortmaker@windriver.com Cc: Namjae Jeon linkinjeon@kernel.org; Steven French Steven.French@microsoft.com; stable@vger.kernel.org Subject: [EXTERNAL] Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport this, just the presence of a random CVE identifier? If that's all it takes to get companies to actually do backports, maybe I should go allocate more of them :)
thanks,
greg k-h
[RE: [EXTERNAL] Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 12/12/2023 (Tue 20:13) Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
There are arguments to be made for larger backports when test infrastructure is good and lots of good functional tests (due to risk of subtle dependencies when cherrypicking 1 patch out of 5 to backport). In general, Namjae has access to excellent functional/regression suites for SMB server (not just from Samba "smbtorture") so it is theoretically possible to do larger "very safe" backports for ksmbd (or at least make these available as an alternative for users who hit serious roadblocks on older kernels), if the test automation were well trusted. Is there a precedent for this?
Larger backports like that can make sense for a target audience who are invested in some feature but don't want to move anything else - then it becomes a deliverable in itself, typically from a professional services group. But linux-stable is definitely not the place for things like that.
Paul. --
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Tuesday, December 12, 2023 2:05 PM To: paul.gortmaker@windriver.com Cc: Namjae Jeon linkinjeon@kernel.org; Steven French Steven.French@microsoft.com; stable@vger.kernel.org Subject: [EXTERNAL] Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport this, just the presence of a random CVE identifier? If that's all it takes to get companies to actually do backports, maybe I should go allocate more of them :)
thanks,
greg k-h
Larger backports like that can make sense for a target audience who are invested in some feature but don't want to move anything else - then it becomes a deliverable in itself, typically from a professional services group. But linux-stable is definitely not the place for things like that.
I wasn't so much thinking about it for stable, but for cases where it is safer to use an alternative to stable for a particular module (and for a particular older kernel). For active components (where dozens of patches go in each release) stable can carry risks because of subtle patch dependencies. Maybe not as much issue for the cifs.ko and ksmbd.ko cases because there are excellent testcase suites that could be automated for stable to catch the vast majority of stable backport problems for those, but that assumes that this (stable kernel component by component testing) is automated (which was a big topic of discussion at the most recent LSF/MM/Storage summit). Obviously distros do this all the time, and backport more than stable would for the more active modules, but there are probably cases where Debian e.g. users would want a module that had various security features backported and so couldn't use stable (because stable would typically not include new features, even if critical)
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
thanks,
greg k-h
2023-12-13 23:36 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
As I know, ksmbd is enable in 5.15 kernel of some distros(opensuse, ubuntu, etc) except redhat. And users can use this feature. I will make the time for ksmbd backporting job. To facilitate backport, Can I submit clean-up patches for ksmbd of 5.15 kernel or only bug fixes are allowed?
Thanks.
thanks,
greg k-h
On Thu, Dec 14, 2023 at 08:31:44AM +0900, Namjae Jeon wrote:
2023-12-13 23:36 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
As I know, ksmbd is enable in 5.15 kernel of some distros(opensuse, ubuntu, etc) except redhat.
But do any of them actually use the 5.15.y kernel tree and take updates from there? That's the key thing here.
And users can use this feature. I will make the time for ksmbd backporting job. To facilitate backport, Can I submit clean-up patches for ksmbd of 5.15 kernel or only bug fixes are allowed?
If a fix relies on an upstream cleanup, that's fine to take.
But first, find out if anyone is actually using this before you take the time here.
thanks,
greg k-h
2023-12-14 17:05 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Thu, Dec 14, 2023 at 08:31:44AM +0900, Namjae Jeon wrote:
2023-12-13 23:36 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
As I know, ksmbd is enable in 5.15 kernel of some distros(opensuse, ubuntu, etc) except redhat.
But do any of them actually use the 5.15.y kernel tree and take updates from there? That's the key thing here.
Yes, openWRT guy said that openWRT use ksmbd module of stable 5.15.y kernel for their NAS function. The most recent major release, 23.05.x, uses the 5.15 kernel, and the kernel version is updated in minor releases. https://github.com/openwrt/openwrt/commit/95ebd609ae7bdcdb48c74ad93d747f24c9...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://github.com/openwrt/openwrt/blob/fcf08d9db6a50a3ca6f0b64d105d975ab896...
And users can use this feature. I will make the time for ksmbd backporting job. To facilitate backport, Can I submit clean-up patches for ksmbd of 5.15 kernel or only bug fixes are allowed?
If a fix relies on an upstream cleanup, that's fine to take.
But first, find out if anyone is actually using this before you take the time here.
Okay:) Thanks for your answer.
thanks,
greg k-h
On Thu, Dec 14, 2023 at 08:33:48PM +0900, Namjae Jeon wrote:
2023-12-14 17:05 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Thu, Dec 14, 2023 at 08:31:44AM +0900, Namjae Jeon wrote:
2023-12-13 23:36 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
As I know, ksmbd is enable in 5.15 kernel of some distros(opensuse, ubuntu, etc) except redhat.
But do any of them actually use the 5.15.y kernel tree and take updates from there? That's the key thing here.
Yes, openWRT guy said that openWRT use ksmbd module of stable 5.15.y kernel for their NAS function. The most recent major release, 23.05.x, uses the 5.15 kernel, and the kernel version is updated in minor releases. https://github.com/openwrt/openwrt/commit/95ebd609ae7bdcdb48c74ad93d747f24c9...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://github.com/openwrt/openwrt/blob/fcf08d9db6a50a3ca6f0b64d105d975ab896...
Ok, thanks, that's good to know. Also you might want to warn them that it's missing loads of security fixes at this point in time and that they might want to move to a newer kernel release :)
thanks,
greg k-h
2023-12-14 20:58 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Thu, Dec 14, 2023 at 08:33:48PM +0900, Namjae Jeon wrote:
2023-12-14 17:05 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Thu, Dec 14, 2023 at 08:31:44AM +0900, Namjae Jeon wrote:
2023-12-13 23:36 GMT+09:00, Greg KH gregkh@linuxfoundation.org:
On Tue, Dec 12, 2023 at 08:13:37PM +0000, Steven French wrote:
Out of curiosity, has there been an alternative approach for some backports, where someone backports most fixes and features (and safe cleanup) but does not backport any of the changesets which have dependencies outside the module (e.g. VFS changes, netfs or mm changes etc.) to reduce patch dependency risk (ie 70-80% backport instead of the typical 10-20% that are picked up by stable)?
For example, we (on the client) ran into issues with 5.15 kernel (for the client) missing so many important fixes and features (and sometimes hard to distinguish when a new feature is also a 'fix') that I did a "full backport" for cifs.ko again a few months ago for 5.15 (leaving out about 10% of the patches, those with dependencies or that would be risky).
We did take a "big backport/sync" for io_uring in 5.15.y a while ago, so there is precident for this.
But really, is anyone even using this feature in 5.15.y anyway? I don't know of any major distro using 5.15.y any more, and Android systems based on 5.15.y don't use this specific filesystem, so what is left? Can we just mark it broken and be done with it?
As I know, ksmbd is enable in 5.15 kernel of some distros(opensuse, ubuntu, etc) except redhat.
But do any of them actually use the 5.15.y kernel tree and take updates from there? That's the key thing here.
Yes, openWRT guy said that openWRT use ksmbd module of stable 5.15.y kernel for their NAS function. The most recent major release, 23.05.x, uses the 5.15 kernel, and the kernel version is updated in minor releases. https://github.com/openwrt/openwrt/commit/95ebd609ae7bdcdb48c74ad93d747f24c9...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://downloads.openwrt.org/releases/23.05.2/targets/x86/64/kmods/5.15.137...
https://github.com/openwrt/openwrt/blob/fcf08d9db6a50a3ca6f0b64d105d975ab896...
Ok, thanks, that's good to know. Also you might want to warn them that it's missing loads of security fixes at this point in time and that they might want to move to a newer kernel release :)
Okay, I will. And I will check ksmbd in 6.1 LTS kernel as well as 5.15. Thanks!
thanks,
greg k-h
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 12/12/2023 (Tue 21:04) Greg KH wrote:
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one
I'd be 100% fine with that. Can't speak for anyone else though.
backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport
I don't use it, and I don't know of anyone who does.
this, just the presence of a random CVE identifier? If that's all it takes to get companies to actually do backports, maybe I should go allocate more of them :)
I wouldn't have normally touched it but someone else was looking at doing the backport and essentially declared it "too hard" and so I couldn't let that stand. :-P
Paul. --
thanks,
greg k-h
On Tue, Dec 12, 2023 at 03:45:55PM -0500, Paul Gortmaker wrote:
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 12/12/2023 (Tue 21:04) Greg KH wrote:
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one
I'd be 100% fine with that. Can't speak for anyone else though.
backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport
I don't use it, and I don't know of anyone who does.
Then why are you all backporting stuff for it?
If no one steps up, I'll just mark the thing as broken, it is _so_ far behind in patches that it's just sad.
thanks,
greg k-h
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 13/12/2023 (Wed 15:34) Greg KH wrote:
On Tue, Dec 12, 2023 at 03:45:55PM -0500, Paul Gortmaker wrote:
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 12/12/2023 (Tue 21:04) Greg KH wrote:
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one
I'd be 100% fine with that. Can't speak for anyone else though.
backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport
I don't use it, and I don't know of anyone who does.
Then why are you all backporting stuff for it?
Firstly, you've cut the context where I already explained that I did it because others said it couldn't be done. Of all people, I am sure you can respect that.
The Yocto Project still offers v5.15 as an option, and whenever I can, I help out to advance the Yocto Project as time permits. Ask Richard.
If no one steps up, I'll just mark the thing as broken, it is _so_ far behind in patches that it's just sad.
Again, in this case - I have no problem with that - but as a note of record -- whenever linux-stable removes a Kconfig, either explicitly or by a depends on BROKEN - it does trigger fallout for some people.
The Yocto/OE does an audit on the Kconfig output looking for options that were explicitly set (or un-set) by the user, or by base templates. If they don't land in the final .config file -- it lets you know.
Paul. --
thanks,
greg k-h
On Wed, Dec 13, 2023 at 10:28:53PM -0500, Paul Gortmaker wrote:
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 13/12/2023 (Wed 15:34) Greg KH wrote:
On Tue, Dec 12, 2023 at 03:45:55PM -0500, Paul Gortmaker wrote:
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 12/12/2023 (Tue 21:04) Greg KH wrote:
On Tue, Dec 12, 2023 at 01:47:44PM -0500, paul.gortmaker@windriver.com wrote:
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.
Nice work, but really, given that there are _SO_ many ksmb patches that have NOT been backported to 5.15.y, I would strongly recommend that we just mark the thing as depending on BROKEN there for now as your one
I'd be 100% fine with that. Can't speak for anyone else though.
backport here is not going to make a dent in the fixes that need to be applied there to resolve the known issues that the codebase currently has resolved in newer kernels.
Do you use this codebase on 5.15.y? What drove you to want to backport
I don't use it, and I don't know of anyone who does.
Then why are you all backporting stuff for it?
Firstly, you've cut the context where I already explained that I did it because others said it couldn't be done. Of all people, I am sure you can respect that.
Sure, I saw that, but I didn't understand why someone was doing it in the first place.
The Yocto Project still offers v5.15 as an option, and whenever I can, I help out to advance the Yocto Project as time permits. Ask Richard.
As an option, but is it recommended and does anyone actually use it there? Does yocto systems expect to use this kernel option for the 5.15 kernel?
If no one steps up, I'll just mark the thing as broken, it is _so_ far behind in patches that it's just sad.
Again, in this case - I have no problem with that - but as a note of record -- whenever linux-stable removes a Kconfig, either explicitly or by a depends on BROKEN - it does trigger fallout for some people.
In what way? Just having to update default config options?
The Yocto/OE does an audit on the Kconfig output looking for options that were explicitly set (or un-set) by the user, or by base templates. If they don't land in the final .config file -- it lets you know.
So defconfig type checks?
thanks,
greg k-h
[Re: [PATCH 0/1] RFC: linux-5.15.y ksmbd backport for CVE-2023-38431] On 14/12/2023 (Thu 09:20) Greg KH wrote:
[...]
If no one steps up, I'll just mark the thing as broken, it is _so_ far behind in patches that it's just sad.
Again, in this case - I have no problem with that - but as a note of record -- whenever linux-stable removes a Kconfig, either explicitly or by a depends on BROKEN - it does trigger fallout for some people.
In what way? Just having to update default config options?
The Yocto/OE does an audit on the Kconfig output looking for options that were explicitly set (or un-set) by the user, or by base templates. If they don't land in the final .config file -- it lets you know.
So defconfig type checks?
Here is a recent example.
https://lists.yoctoproject.org/g/linux-yocto/message/13387
I am not saying linux-stable is wrong to do these kinds of changes, I'm just saying there is an impact that people might not be aware of.
Paul. --
thanks,
greg k-h
2023-12-13 3:47 GMT+09:00, paul.gortmaker@windriver.com paul.gortmaker@windriver.com:
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_hdrgit 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
linux-stable-mirror@lists.linaro.org
-
Greg KH -
Namjae Jeon -
Paul Gortmaker -
paul.gortmaker＠windriver.com
-
Steven French