Skip to contentSkip to conversation

sctp: avoid auth_enable sysctl UAF during netns teardown

PATCH Superseded Ren Wei 2 participants 3 messages Jul 11 – Jul 13 1 message not in the archive

Subsystems (3)
Networking Network drivers SCTP protocol

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

Ren Wei

author
#1 lore raw

PATCH v2 1/2 sctp: avoid auth_enable sysctl UAF during netns teardown

From: Zhiling Zou <[email protected]>

proc_sctp_do_auth() updates the SCTP control socket after changing net.sctp.auth_enable. The handler gets the per-net SCTP state from ctl->data, so an already opened sysctl file can still target a network namespace while that namespace is being torn down.

SCTP previously registered its per-net sysctls from sctp_defaults_init(), while the control socket is created later from sctp_ctrlsock_init(). This exposed a window during initialization where auth_enable was writable before net->sctp.ctl_sock existed, and a teardown window where auth_enable stayed writable after inet_ctl_sock_destroy() had released the control socket.

Move the per-net SCTP sysctl registration into sctp_ctrlsock_init() after sctp_ctl_sock_init() succeeds, and unregister the sysctl table before destroying the control socket in sctp_ctrlsock_exit(). If sysctl registration fails after the control socket was created, destroy the control socket in the same init path.

Make sctp_sysctl_net_unregister() tolerate a missing header and clear the saved pointer so init-error and exit paths can safely share the unregister helper.

Fixes 15649fd5415e ("sctp: sysctl: auth_enable: avoid using current->nsproxy")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Co-developed-by Qi Tang <[email protected]>
Signed-off-by Qi Tang <[email protected]>
Signed-off-by Zhiling Zou <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 net/sctp/protocol.c | 17 ++++++++++-------
 net/sctp/sysctl.c   |  9 +++++++--
 2 files changed, 17 insertions(+), 9 deletions(-)
net/sctp/protocol.c +10 -7
index 587b0017a67d..f5fe6ddf0d7d 100644
@@ -1382,10 +1382,6 @@ static int __net_init sctp_defaults_init(struct net *net)
 	net->sctp.l3mdev_accept = 1;
 #endif
 
-	status = sctp_sysctl_net_register(net);
-	if (status)
-		goto err_sysctl_register;
-
 	/* Allocate and initialise sctp mibs.  */
 	status = init_sctp_mibs(net);
 	if (status)

@@ -1419,8 +1415,6 @@ static int __net_init sctp_defaults_init(struct net *net)
 	cleanup_sctp_mibs(net);
 #endif
 err_init_mibs:
-	sctp_sysctl_net_unregister(net);
-err_sysctl_register:
 	return status;
 }
 

@@ -1435,7 +1429,6 @@ static void __net_exit sctp_defaults_exit(struct net *net)
 	net->sctp.proc_net_sctp = NULL;
 #endif
 	cleanup_sctp_mibs(net);
-	sctp_sysctl_net_unregister(net);
 }
 
 static struct pernet_operations sctp_defaults_ops = {

@@ -1451,14 +1444,24 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
 	status = sctp_ctl_sock_init(net);
 	if (status)
 		pr_err("Failed to initialize the SCTP control sock\n");
+	else
+		status = sctp_sysctl_net_register(net);
+
+	if (status && net->sctp.ctl_sock) {
+		inet_ctl_sock_destroy(net->sctp.ctl_sock);
+		net->sctp.ctl_sock = NULL;
+	}
 
 	return status;
 }
 
 static void __net_exit sctp_ctrlsock_exit(struct net *net)
 {
+	sctp_sysctl_net_unregister(net);
+
 	/* Free the control endpoint.  */
 	inet_ctl_sock_destroy(net->sctp.ctl_sock);
+	net->sctp.ctl_sock = NULL;
 }
 
 static struct pernet_operations sctp_ctrlsock_ops = {
net/sctp/sysctl.c +7 -2
index 15e7db9a3ab2..fca840484ebf 100644
@@ -615,11 +615,16 @@ int sctp_sysctl_net_register(struct net *net)
 
 void sctp_sysctl_net_unregister(struct net *net)
 {
+	struct ctl_table_header *header = net->sctp.sysctl_header;
 	const struct ctl_table *table;
 
-	table = net->sctp.sysctl_header->ctl_table_arg;
-	unregister_net_sysctl_table(net->sctp.sysctl_header);
+	if (!header)
+		return;
+
+	table = header->ctl_table_arg;
+	unregister_net_sysctl_table(header);
 	kfree(table);
+	net->sctp.sysctl_header = NULL;
 }
 
 static struct ctl_table_header *sctp_sysctl_header;
Signature · 1 line
2.43.0
maintainer
#2 lore raw
Quoted 73 lines · Ren Wei

From: Zhiling Zou <[email protected]>

proc_sctp_do_auth() updates the SCTP control socket after changing net.sctp.auth_enable. The handler gets the per-net SCTP state from ctl->data, so an already opened sysctl file can still target a network namespace while that namespace is being torn down.

SCTP previously registered its per-net sysctls from sctp_defaults_init(), while the control socket is created later from sctp_ctrlsock_init(). This exposed a window during initialization where auth_enable was writable before net->sctp.ctl_sock existed, and a teardown window where auth_enable stayed writable after inet_ctl_sock_destroy() had released the control socket.

Move the per-net SCTP sysctl registration into sctp_ctrlsock_init() after sctp_ctl_sock_init() succeeds, and unregister the sysctl table before destroying the control socket in sctp_ctrlsock_exit(). If sysctl registration fails after the control socket was created, destroy the control socket in the same init path.

Make sctp_sysctl_net_unregister() tolerate a missing header and clear the saved pointer so init-error and exit paths can safely share the unregister helper.

Fixes 15649fd5415e ("sctp: sysctl: auth_enable: avoid using current->nsproxy")
Cc [email protected]
Reported-by Yuan Tan <[email protected]>
Reported-by Yifan Wu <[email protected]>
Reported-by Juefei Pu <[email protected]>
Reported-by Xin Liu <[email protected]>
Co-developed-by Qi Tang <[email protected]>
Signed-off-by Qi Tang <[email protected]>
Signed-off-by Zhiling Zou <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 net/sctp/protocol.c | 17 ++++++++++-------
 net/sctp/sysctl.c   |  9 +++++++--
 2 files changed, 17 insertions(+), 9 deletions(-)
net/sctp/protocol.c +0 -7
index 587b0017a67d..f5fe6ddf0d7d 100644
@@ -1382,10 +1382,6 @@ static int __net_init sctp_defaults_init(struct net *net)
        net->sctp.l3mdev_accept = 1;
 #endif
 
-       status = sctp_sysctl_net_register(net);
-       if (status)
-               goto err_sysctl_register;
-
        /* Allocate and initialise sctp mibs.  */
        status = init_sctp_mibs(net);
        if (status)

@@ -1419,8 +1415,6 @@ static int __net_init sctp_defaults_init(struct net *net)
        cleanup_sctp_mibs(net);
 #endif
 err_init_mibs:
-       sctp_sysctl_net_unregister(net);
-err_sysctl_register:
        return status;
 }
 

@@ -1435,7 +1429,6 @@ static void __net_exit sctp_defaults_exit(struct net *net)
        net->sctp.proc_net_sctp = NULL;
 #endif
        cleanup_sctp_mibs(net);
-       sctp_sysctl_net_unregister(net);
 }
 
 static struct pernet_operations sctp_defaults_ops = {

net/sctp/protocol.c static int __net_init sctp_ctrlsock_init(struct net *net) · line 1453

Quoted 10 lines
net/sctp/protocol.c +7 -0
@@ -1451,14 +1444,24 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
        status = sctp_ctl_sock_init(net);
        if (status)
                pr_err("Failed to initialize the SCTP control sock\n");
+       else
+               status = sctp_sysctl_net_register(net);
+
+       if (status && net->sctp.ctl_sock) {
+               inet_ctl_sock_destroy(net->sctp.ctl_sock);
+               net->sctp.ctl_sock = NULL;
+       }

I think the Linux style here should be:

        /* Initialize the control inode/socket for handling OOTB packets.  */
        status = sctp_ctl_sock_init(net);
        if (status) {
                pr_err("Failed to initialize the SCTP control sock\n");
                return status;
        }

        status = sctp_sysctl_net_register(net);
        if (status) {
                inet_ctl_sock_destroy(net->sctp.ctl_sock);
                net->sctp.ctl_sock = NULL;
        }

Thanks.

Quoted 40 lines
net/sctp/protocol.c +3 -0
        return status;
 }
 
 static void __net_exit sctp_ctrlsock_exit(struct net *net)
 {
+       sctp_sysctl_net_unregister(net);
+
        /* Free the control endpoint.  */
        inet_ctl_sock_destroy(net->sctp.ctl_sock);
+       net->sctp.ctl_sock = NULL;
 }
 
 static struct pernet_operations sctp_ctrlsock_ops = {
net/sctp/sysctl.c +7 -3
index 15e7db9a3ab2..fca840484ebf 100644
@@ -615,11 +615,16 @@ int sctp_sysctl_net_register(struct net *net)
 
 void sctp_sysctl_net_unregister(struct net *net)
 {
+       struct ctl_table_header *header = net->sctp.sysctl_header;
        const struct ctl_table *table;
 
-       table = net->sctp.sysctl_header->ctl_table_arg;
-       unregister_net_sysctl_table(net->sctp.sysctl_header);
+       if (!header)
+               return;
+
+       table = header->ctl_table_arg;
+       unregister_net_sysctl_table(header);
        kfree(table);
+       net->sctp.sysctl_header = NULL;
 }
 
 static struct ctl_table_header *sctp_sysctl_header;
--

2.43.0

author
#3 lore raw

PATCH v2 2/2 sctp: close UDP tunnel sockets during netns teardown

In reply to a message not in the archive

From: Zhiling Zou <[email protected]>

proc_sctp_do_udp_port() starts per-net SCTP UDP tunneling sockets when net.sctp.udp_port is set, and stops/restarts them when the sysctl value changes. The netns exit path does not stop these sockets, so a namespace can be torn down while its SCTP UDP tunnel sockets are still installed.

Close the UDP tunnel sockets from sctp_ctrlsock_exit() after unregistering the per-net sysctl table. This prevents new sysctl writes from racing in while the sockets are being released, and closes the sockets before the control socket is destroyed.

Fixes 046c052b475e ("sctp: enable udp tunneling socks")
Cc [email protected]
Reported-by Sashiko <[email protected]>
Closes https://sashiko.dev/#/patchset/b9f1f02b0780ad6a719e2413f5f0bb8eb7702d94.1782585631.git.roxy520tt%40gmail.com
Signed-off-by Zhiling Zou <[email protected]>
Signed-off-by Ren Wei <[email protected]>
---
 net/sctp/protocol.c | 1 +
 1 file changed, 1 insertion(+)
net/sctp/protocol.c +1 -0
index f5fe6ddf0d7d..5dbc7e9e8f06 100644
@@ -1458,6 +1458,7 @@ static int __net_init sctp_ctrlsock_init(struct net *net)
 static void __net_exit sctp_ctrlsock_exit(struct net *net)
 {
 	sctp_sysctl_net_unregister(net);
+	sctp_udp_sock_stop(net);
 
 	/* Free the control endpoint.  */
 	inet_ctl_sock_destroy(net->sctp.ctl_sock);
Signature · 1 line
2.43.0