| Message ID | 20250402200319.2834-2-kuniyu@amazon.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cifs: Revert bogus fix for CVE-2024-54680 and its followup commit. | expand |
Acked-by: Wang Zhaolong <wangzhaolong1@huawei.com> > This reverts commit 4e7f1644f2ac6d01dc584f6301c3b1d5aac4eaef. > > The commit e9f2517a3e18 ("smb: client: fix TCP timers deadlock after > rmmod") is not only a bogus fix for LOCKDEP null-ptr-deref but also > introduces a real issue, TCP sockets leak, which will be explained in > detail in the next revert. > > Also, CNA assigned CVE-2024-54680 to it but is rejecting it. [0] > > Thus, we are reverting the commit and its follow-up commit 4e7f1644f2ac > ("smb: client: Fix netns refcount imbalance causing leaks and > use-after-free"). > > Link: https://lore.kernel.org/all/2025040248-tummy-smilingly-4240@gregkh/ #[0] > Fixes: 4e7f1644f2ac ("smb: client: Fix netns refcount imbalance causing leaks and use-after-free") > Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> > --- > fs/smb/client/connect.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c > index 10a7c28d2d44..137a611c5ab0 100644 > --- a/fs/smb/client/connect.c > +++ b/fs/smb/client/connect.c > @@ -300,7 +300,6 @@ cifs_abort_connection(struct TCP_Server_Info *server) > server->ssocket->flags); > sock_release(server->ssocket); > server->ssocket = NULL; > - put_net(cifs_net_ns(server)); > } > server->sequence_number = 0; > server->session_estab = false; > @@ -3367,12 +3366,8 @@ generic_ip_connect(struct TCP_Server_Info *server) > /* > * Grab netns reference for the socket. > * > - * This reference will be released in several situations: > - * - In the failure path before the cifsd thread is started. > - * - In the all place where server->socket is released, it is > - * also set to NULL. > - * - Ultimately in clean_demultiplex_info(), during the final > - * teardown. > + * It'll be released here, on error, or in clean_demultiplex_info() upon server > + * teardown. > */ > get_net(net); > > @@ -3388,8 +3383,10 @@ generic_ip_connect(struct TCP_Server_Info *server) > } > > rc = bind_socket(server); > - if (rc < 0) > + if (rc < 0) { > + put_net(cifs_net_ns(server)); > return rc; > + } > > /* > * Eventually check for other socket options to change from > @@ -3444,6 +3441,9 @@ generic_ip_connect(struct TCP_Server_Info *server) > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > rc = ip_rfc1001_connect(server); > > + if (rc < 0) > + put_net(cifs_net_ns(server)); > + > return rc; > } >
Hi Kuniyuki, When testing this patch on the latest mainline, I found that the following snippet has a conflict: > @@ -3444,6 +3441,9 @@ generic_ip_connect(struct TCP_Server_Info *server) > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > rc = ip_rfc1001_connect(server); > > + if (rc < 0) > + put_net(cifs_net_ns(server)); > + > return rc; > } > Specifically, it is this line: > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) In my code, it corresponds to the following snippet: ``` @@ -3333,10 +3330,13 @@ generic_ip_connect(struct TCP_Server_Info *server) } trace_smb3_connect_done(server->hostname, server->conn_id, &server->dstaddr); if (sport == htons(RFC1001_PORT)) rc = ip_rfc1001_connect(server); + if (rc < 0) + put_net(cifs_net_ns(server)); + return rc; } ``` Looks like V3 needs to be sent? Best regards, Wang Zhaolong
From: Wang Zhaolong <wangzhaolong1@huawei.com> Date: Thu, 3 Apr 2025 17:59:20 +0800 > Hi Kuniyuki, > > When testing this patch on the latest mainline, I found that the following > snippet has a conflict: I guess it's because I used for-next branch of the cifs.git. Steve: What branch should be used to send reverts for -rcX ? > > > > @@ -3444,6 +3441,9 @@ generic_ip_connect(struct TCP_Server_Info *server) > > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > > rc = ip_rfc1001_connect(server); > > > > + if (rc < 0) > > + put_net(cifs_net_ns(server)); > > + > > return rc; > > } > > > > Specifically, it is this line: > > > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > > In my code, it corresponds to the following snippet: > > ``` > @@ -3333,10 +3330,13 @@ generic_ip_connect(struct TCP_Server_Info *server) > } > trace_smb3_connect_done(server->hostname, server->conn_id, &server->dstaddr); > if (sport == htons(RFC1001_PORT)) > rc = ip_rfc1001_connect(server); > > + if (rc < 0) > + put_net(cifs_net_ns(server)); > + > return rc; > } > ``` > > Looks like V3 needs to be sent? > > Best regards, > Wang Zhaolong
> What branch should be used to send reverts for -rcX ? cifs-2.6.git for-next But probably won't be an issue in a few days (since mainline then will likely include the conflicting patch - which fortunately is unrelated to this discussion, just fixes an RFC1001 bug) On Thu, Apr 3, 2025 at 12:27 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote: > > From: Wang Zhaolong <wangzhaolong1@huawei.com> > Date: Thu, 3 Apr 2025 17:59:20 +0800 > > Hi Kuniyuki, > > > > When testing this patch on the latest mainline, I found that the following > > snippet has a conflict: > > I guess it's because I used for-next branch of the cifs.git. > > Steve: > > What branch should be used to send reverts for -rcX ? > > > > > > > > > @@ -3444,6 +3441,9 @@ generic_ip_connect(struct TCP_Server_Info *server) > > > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > > > rc = ip_rfc1001_connect(server); > > > > > > + if (rc < 0) > > > + put_net(cifs_net_ns(server)); > > > + > > > return rc; > > > } > > > > > > > Specifically, it is this line: > > > > > (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) > > > > In my code, it corresponds to the following snippet: > > > > ``` > > @@ -3333,10 +3330,13 @@ generic_ip_connect(struct TCP_Server_Info *server) > > } > > trace_smb3_connect_done(server->hostname, server->conn_id, &server->dstaddr); > > if (sport == htons(RFC1001_PORT)) > > rc = ip_rfc1001_connect(server); > > > > + if (rc < 0) > > + put_net(cifs_net_ns(server)); > > + > > return rc; > > } > > ``` > > > > Looks like V3 needs to be sent? > > > > Best regards, > > Wang Zhaolong >
From: Steve French <smfrench@gmail.com> Date: Thu, 3 Apr 2025 12:32:35 -0500 > > What branch should be used to send reverts for -rcX ? > > cifs-2.6.git for-next > > But probably won't be an issue in a few days (since mainline then will > likely include the conflicting patch - which fortunately is unrelated > to this discussion, just fixes an RFC1001 bug) Thanks, then I'd leave v2 as is for now.
diff --git a/fs/smb/client/connect.c b/fs/smb/client/connect.c index 10a7c28d2d44..137a611c5ab0 100644 --- a/fs/smb/client/connect.c +++ b/fs/smb/client/connect.c @@ -300,7 +300,6 @@ cifs_abort_connection(struct TCP_Server_Info *server) server->ssocket->flags); sock_release(server->ssocket); server->ssocket = NULL; - put_net(cifs_net_ns(server)); } server->sequence_number = 0; server->session_estab = false; @@ -3367,12 +3366,8 @@ generic_ip_connect(struct TCP_Server_Info *server) /* * Grab netns reference for the socket. * - * This reference will be released in several situations: - * - In the failure path before the cifsd thread is started. - * - In the all place where server->socket is released, it is - * also set to NULL. - * - Ultimately in clean_demultiplex_info(), during the final - * teardown. + * It'll be released here, on error, or in clean_demultiplex_info() upon server + * teardown. */ get_net(net); @@ -3388,8 +3383,10 @@ generic_ip_connect(struct TCP_Server_Info *server) } rc = bind_socket(server); - if (rc < 0) + if (rc < 0) { + put_net(cifs_net_ns(server)); return rc; + } /* * Eventually check for other socket options to change from @@ -3444,6 +3441,9 @@ generic_ip_connect(struct TCP_Server_Info *server) (server->rfc1001_sessinit == -1 && sport == htons(RFC1001_PORT))) rc = ip_rfc1001_connect(server); + if (rc < 0) + put_net(cifs_net_ns(server)); + return rc; }
This reverts commit 4e7f1644f2ac6d01dc584f6301c3b1d5aac4eaef. The commit e9f2517a3e18 ("smb: client: fix TCP timers deadlock after rmmod") is not only a bogus fix for LOCKDEP null-ptr-deref but also introduces a real issue, TCP sockets leak, which will be explained in detail in the next revert. Also, CNA assigned CVE-2024-54680 to it but is rejecting it. [0] Thus, we are reverting the commit and its follow-up commit 4e7f1644f2ac ("smb: client: Fix netns refcount imbalance causing leaks and use-after-free"). Link: https://lore.kernel.org/all/2025040248-tummy-smilingly-4240@gregkh/ #[0] Fixes: 4e7f1644f2ac ("smb: client: Fix netns refcount imbalance causing leaks and use-after-free") Signed-off-by: Kuniyuki Iwashima <kuniyu@amazon.com> --- fs/smb/client/connect.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)