[PATCH net-next v3 0/9] do not leave dangling sk pointers in pf->create functions

[PATCH net-next v3 0/9] do not leave dangling sk pointers in pf->create functions

[Ignat Korchagin]

Some protocol family create() implementations have an error path after
allocating the sk object and calling sock_init_data(). sock_init_data()
attaches the allocated sk object to the sock object, provided by the
caller.

If the create() implementation errors out after calling sock_init_data(),
it releases the allocated sk object, but the caller ends up having a
dangling sk pointer in its sock object on return. Subsequent manipulations
on this sock object may try to access the sk pointer, because it is not
NULL thus creating a use-after-free scenario.

We have implemented a stable hotfix in commit 631083143315
("net: explicitly clear the sk pointer, when pf->create fails"), but this
series aims to fix it properly by going through each of the pf->create()
implementations and making sure they all don't return a sock object with
a dangling pointer on error.

Changes in V3:
  * retargeted the series to net-next
  * dropped the hotfix patch, which was merged into net already
  * replaced the hotfix code with DEBUG_NET_WARN_ON_ONCE() to catch future
    violations

Changes in V2:
  * reverted the change introduced in 6cd4a78d962b ("net: do not leave a
    dangling sk pointer, when socket creation fails")
  * added optional commits to all pf->create implementaions to clear the
    sk pointer on error after sock_init_data()

Ignat Korchagin (9):
  af_packet: avoid erroring out after sock_init_data() in
    packet_create()
  Bluetooth: L2CAP: do not leave dangling sk pointer on error in
    l2cap_sock_create()
  Bluetooth: RFCOMM: avoid leaving dangling sk pointer in
    rfcomm_sock_alloc()
  net: af_can: do not leave a dangling sk pointer in can_create()
  net: ieee802154: do not leave a dangling sk pointer in
    ieee802154_create()
  net: inet: do not leave a dangling sk pointer in inet_create()
  net: inet6: do not leave a dangling sk pointer in inet6_create()
  net: warn, if pf->create does not clear sock->sk on error
  Revert "net: do not leave a dangling sk pointer, when socket creation
    fails"

 net/bluetooth/l2cap_sock.c  |  1 +
 net/bluetooth/rfcomm/sock.c | 10 +++++-----
 net/can/af_can.c            |  1 +
 net/core/sock.c             |  3 ---
 net/ieee802154/socket.c     | 12 +++++++-----
 net/ipv4/af_inet.c          | 22 ++++++++++------------
 net/ipv6/af_inet6.c         | 22 ++++++++++------------
 net/packet/af_packet.c      | 12 ++++++------
 net/socket.c                |  4 ++--
 9 files changed, 42 insertions(+), 45 deletions(-)

-- 
2.39.5

[PATCH net-next v3 1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()

[Ignat Korchagin]

After sock_init_data() the allocated sk object is attached to the provided
sock object. On error, packet_create() frees the sk object leaving the
dangling pointer in the sock object on return. Some other code may try
to use this pointer and cause use-after-free.

Suggested-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/packet/af_packet.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index f8942062f776..99ae27d1e4dc 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -3421,17 +3421,17 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
 	if (sock->type == SOCK_PACKET)
 		sock->ops = &packet_ops_spkt;
 
+	po = pkt_sk(sk);
+	err = packet_alloc_pending(po);
+	if (err)
+		goto out_sk_free;
+
 	sock_init_data(sock, sk);
 
-	po = pkt_sk(sk);
 	init_completion(&po->skb_completion);
 	sk->sk_family = PF_PACKET;
 	po->num = proto;
 
-	err = packet_alloc_pending(po);
-	if (err)
-		goto out2;
-
 	packet_cached_dev_reset(po);
 
 	sk->sk_destruct = packet_sock_destruct;
@@ -3463,7 +3463,7 @@ static int packet_create(struct net *net, struct socket *sock, int protocol,
 	sock_prot_inuse_add(net, &packet_proto, 1);
 
 	return 0;
-out2:
+out_sk_free:
 	sk_free(sk);
 out:
 	return err;
-- 
2.39.5

[PATCH net-next v3 2/9] Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()

[Ignat Korchagin]

bt_sock_alloc() allocates the sk object and attaches it to the provided
sock object. On error l2cap_sock_alloc() frees the sk object, but the
dangling pointer is still attached to the sock object, which may create
use-after-free in other code.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/bluetooth/l2cap_sock.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index ba437c6f6ee5..18e89e764f3b 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1886,6 +1886,7 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
 	chan = l2cap_chan_create();
 	if (!chan) {
 		sk_free(sk);
+		sock->sk = NULL;
 		return NULL;
 	}
 
-- 
2.39.5

[PATCH net-next v3 3/9] Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()

[Ignat Korchagin]

bt_sock_alloc() attaches allocated sk object to the provided sock object.
If rfcomm_dlc_alloc() fails, we release the sk object, but leave the
dangling pointer in the sock object, which may cause use-after-free.

Fix this by swapping calls to bt_sock_alloc() and rfcomm_dlc_alloc().

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/bluetooth/rfcomm/sock.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index f48250e3f2e1..355e1a1698f5 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -274,13 +274,13 @@ static struct sock *rfcomm_sock_alloc(struct net *net, struct socket *sock,
 	struct rfcomm_dlc *d;
 	struct sock *sk;
 
-	sk = bt_sock_alloc(net, sock, &rfcomm_proto, proto, prio, kern);
-	if (!sk)
+	d = rfcomm_dlc_alloc(prio);
+	if (!d)
 		return NULL;
 
-	d = rfcomm_dlc_alloc(prio);
-	if (!d) {
-		sk_free(sk);
+	sk = bt_sock_alloc(net, sock, &rfcomm_proto, proto, prio, kern);
+	if (!sk) {
+		rfcomm_dlc_free(d);
 		return NULL;
 	}
 
-- 
2.39.5

[PATCH net-next v3 4/9] net: af_can: do not leave a dangling sk pointer in can_create()

[Ignat Korchagin]

On error can_create() frees the allocated sk object, but sock_init_data()
has already attached it to the provided sock object. This will leave a
dangling sk pointer in the sock object and may cause use-after-free later.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
---
 net/can/af_can.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/can/af_can.c b/net/can/af_can.c
index 707576eeeb58..01f3fbb3b67d 100644
--- a/net/can/af_can.c
+++ b/net/can/af_can.c
@@ -171,6 +171,7 @@ static int can_create(struct net *net, struct socket *sock, int protocol,
 		/* release sk on errors */
 		sock_orphan(sk);
 		sock_put(sk);
+		sock->sk = NULL;
 	}
 
  errout:
-- 
2.39.5

[PATCH net-next v3 5/9] net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()

[Ignat Korchagin]

sock_init_data() attaches the allocated sk object to the provided sock
object. If ieee802154_create() fails later, the allocated sk object is
freed, but the dangling pointer remains in the provided sock object, which
may allow use-after-free.

Clear the sk pointer in the sock object on error.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 net/ieee802154/socket.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
index 990a83455dcf..18d267921bb5 100644
--- a/net/ieee802154/socket.c
+++ b/net/ieee802154/socket.c
@@ -1043,19 +1043,21 @@ static int ieee802154_create(struct net *net, struct socket *sock,
 
 	if (sk->sk_prot->hash) {
 		rc = sk->sk_prot->hash(sk);
-		if (rc) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (rc)
+			goto out_sk_release;
 	}
 
 	if (sk->sk_prot->init) {
 		rc = sk->sk_prot->init(sk);
 		if (rc)
-			sk_common_release(sk);
+			goto out_sk_release;
 	}
 out:
 	return rc;
+out_sk_release:
+	sk_common_release(sk);
+	sock->sk = NULL;
+	goto out;
 }
 
 static const struct net_proto_family ieee802154_family_ops = {
-- 
2.39.5

[PATCH net-next v3 6/9] net: inet: do not leave a dangling sk pointer in inet_create()

[Ignat Korchagin]

sock_init_data() attaches the allocated sk object to the provided sock
object. If inet_create() fails later, the sk object is freed, but the
sock object retains the dangling pointer, which may create use-after-free
later.

Clear the sk pointer in the sock object on error.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/ipv4/af_inet.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index b24d74616637..8095e82de808 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -376,32 +376,30 @@ static int inet_create(struct net *net, struct socket *sock, int protocol,
 		inet->inet_sport = htons(inet->inet_num);
 		/* Add to protocol hash chains. */
 		err = sk->sk_prot->hash(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 
 	if (sk->sk_prot->init) {
 		err = sk->sk_prot->init(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 
 	if (!kern) {
 		err = BPF_CGROUP_RUN_PROG_INET_SOCK(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 out:
 	return err;
 out_rcu_unlock:
 	rcu_read_unlock();
 	goto out;
+out_sk_release:
+	sk_common_release(sk);
+	sock->sk = NULL;
+	goto out;
 }
 
 
-- 
2.39.5

[PATCH net-next v3 7/9] net: inet6: do not leave a dangling sk pointer in inet6_create()

[Ignat Korchagin]

sock_init_data() attaches the allocated sk pointer to the provided sock
object. If inet6_create() fails later, the sk object is released, but the
sock object retains the dangling sk pointer, which may cause use-after-free
later.

Clear the sock sk pointer on error.

Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/ipv6/af_inet6.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
index ba69b86f1c7d..f60ec8b0f8ea 100644
--- a/net/ipv6/af_inet6.c
+++ b/net/ipv6/af_inet6.c
@@ -252,31 +252,29 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
 		 */
 		inet->inet_sport = htons(inet->inet_num);
 		err = sk->sk_prot->hash(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 	if (sk->sk_prot->init) {
 		err = sk->sk_prot->init(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 
 	if (!kern) {
 		err = BPF_CGROUP_RUN_PROG_INET_SOCK(sk);
-		if (err) {
-			sk_common_release(sk);
-			goto out;
-		}
+		if (err)
+			goto out_sk_release;
 	}
 out:
 	return err;
 out_rcu_unlock:
 	rcu_read_unlock();
 	goto out;
+out_sk_release:
+	sk_common_release(sk);
+	sock->sk = NULL;
+	goto out;
 }
 
 static int __inet6_bind(struct sock *sk, struct sockaddr *uaddr, int addr_len,
-- 
2.39.5

[PATCH net-next v3 8/9] net: warn, if pf->create does not clear sock->sk on error

[Ignat Korchagin]

All pf->create implementations have been fixed now to clear sock->sk on
error, when they deallocate the allocated sk object.

Put a warning in place to make sure we don't break this promise in the
future.

Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/socket.c b/net/socket.c
index 24b404299015..9a8e4452b9b2 100644
--- a/net/socket.c
+++ b/net/socket.c
@@ -1576,9 +1576,9 @@ int __sock_create(struct net *net, int family, int type, int protocol,
 	err = pf->create(net, sock, protocol, kern);
 	if (err < 0) {
 		/* ->create should release the allocated sock->sk object on error
-		 * but it may leave the dangling pointer
+		 * and make sure sock->sk is set to NULL to avoid use-after-free
 		 */
-		sock->sk = NULL;
+		DEBUG_NET_WARN_ON_ONCE(sock->sk);
 		goto out_module_put;
 	}
 
-- 
2.39.5

[PATCH net-next v3 9/9] Revert "net: do not leave a dangling sk pointer, when socket creation fails"

[Ignat Korchagin]

This reverts commit 6cd4a78d962bebbaf8beb7d2ead3f34120e3f7b2.

inet/inet6->create() implementations have been fixed to explicitly NULL the
allocated sk object on error.

A warning was put in place to make sure any future changes will not leave
a dangling pointer in pf->create() implementations.

So this code is now redundant.

Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
---
 net/core/sock.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 083d438d8b6f..a9391cb796a2 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -3830,9 +3830,6 @@ void sk_common_release(struct sock *sk)
 
 	sk->sk_prot->unhash(sk);
 
-	if (sk->sk_socket)
-		sk->sk_socket->sk = NULL;
-
 	/*
 	 * In this point socket cannot receive new packets, but it is possible
 	 * that some packets are in flight because some CPU runs receiver and
-- 
2.39.5

RE: do not leave dangling sk pointers in pf->create functions

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

This is an automated email and please do not reply to this email.

Dear Submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.

----- Output -----

error: patch failed: net/socket.c:1576
error: net/socket.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch

Please resolve the issue and submit the patches again.


---
Regards,
Linux Bluetooth

Re: [PATCH net-next v3 1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:00 +0100
> After sock_init_data() the allocated sk object is attached to the provided
> sock object. On error, packet_create() frees the sk object leaving the
> dangling pointer in the sock object on return. Some other code may try
> to use this pointer and cause use-after-free.
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 2/9] Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:01 +0100
> bt_sock_alloc() allocates the sk object and attaches it to the provided
> sock object. On error l2cap_sock_alloc() frees the sk object, but the
> dangling pointer is still attached to the sock object, which may create
> use-after-free in other code.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Checked all bt_sock_alloc() paths and confirmed only rfcomm and l2cap
need changes.

Re: [PATCH net-next v3 3/9] Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:02 +0100
> bt_sock_alloc() attaches allocated sk object to the provided sock object.
> If rfcomm_dlc_alloc() fails, we release the sk object, but leave the
> dangling pointer in the sock object, which may cause use-after-free.
> 
> Fix this by swapping calls to bt_sock_alloc() and rfcomm_dlc_alloc().
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 4/9] net: af_can: do not leave a dangling sk pointer in can_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:03 +0100
> On error can_create() frees the allocated sk object, but sock_init_data()
> has already attached it to the provided sock object. This will leave a
> dangling sk pointer in the sock object and may cause use-after-free later.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 5/9] net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:04 +0100
> sock_init_data() attaches the allocated sk object to the provided sock
> object. If ieee802154_create() fails later, the allocated sk object is
> freed, but the dangling pointer remains in the provided sock object, which
> may allow use-after-free.
> 
> Clear the sk pointer in the sock object on error.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 6/9] net: inet: do not leave a dangling sk pointer in inet_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:05 +0100
> sock_init_data() attaches the allocated sk object to the provided sock
> object. If inet_create() fails later, the sk object is freed, but the
> sock object retains the dangling pointer, which may create use-after-free
> later.
> 
> Clear the sk pointer in the sock object on error.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 7/9] net: inet6: do not leave a dangling sk pointer in inet6_create()

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:06 +0100
> sock_init_data() attaches the allocated sk pointer to the provided sock
> object. If inet6_create() fails later, the sk object is released, but the
> sock object retains the dangling sk pointer, which may cause use-after-free
> later.
> 
> Clear the sock sk pointer on error.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 8/9] net: warn, if pf->create does not clear sock->sk on error

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:07 +0100
> All pf->create implementations have been fixed now to clear sock->sk on
> error, when they deallocate the allocated sk object.
> 
> Put a warning in place to make sure we don't break this promise in the
> future.
> 
> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 9/9] Revert "net: do not leave a dangling sk pointer, when socket creation fails"

[Kuniyuki Iwashima]

From: Ignat Korchagin <ignat@cloudflare.com>
Date: Mon, 14 Oct 2024 16:38:08 +0100
> This reverts commit 6cd4a78d962bebbaf8beb7d2ead3f34120e3f7b2.
> 
> inet/inet6->create() implementations have been fixed to explicitly NULL the
> allocated sk object on error.
> 
> A warning was put in place to make sure any future changes will not leave
> a dangling pointer in pf->create() implementations.
> 
> So this code is now redundant.
> 
> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Re: [PATCH net-next v3 1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()

[Willem de Bruijn]

Ignat Korchagin wrote:
> After sock_init_data() the allocated sk object is attached to the provided
> sock object. On error, packet_create() frees the sk object leaving the
> dangling pointer in the sock object on return. Some other code may try
> to use this pointer and cause use-after-free.
> 
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next v3 4/9] net: af_can: do not leave a dangling sk pointer in can_create()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On 14.10.2024 16:38:03, Ignat Korchagin wrote:
> On error can_create() frees the allocated sk object, but sock_init_data()
> has already attached it to the provided sock object. This will leave a
> dangling sk pointer in the sock object and may cause use-after-free later.
> 
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>

Reviewed-by: Marc Kleine-Budde <mkl@pengutronix.de>

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH net-next v3 1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 5:38 PM Ignat Korchagin <ignat@cloudflare.com> wrote:
>
> After sock_init_data() the allocated sk object is attached to the provided
> sock object. On error, packet_create() frees the sk object leaving the
> dangling pointer in the sock object on return. Some other code may try
> to use this pointer and cause use-after-free.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> ---

Reviewed-by: Eric Dumazet <edumazet@google.com>

Thanks.

Re: [PATCH net-next v3 2/9] Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:23 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:01 +0100
> > bt_sock_alloc() allocates the sk object and attaches it to the provided
> > sock object. On error l2cap_sock_alloc() frees the sk object, but the
> > dangling pointer is still attached to the sock object, which may create
> > use-after-free in other code.
> >
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>
>
> Checked all bt_sock_alloc() paths and confirmed only rfcomm and l2cap
> need changes.

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 3/9] Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:30 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:02 +0100
> > bt_sock_alloc() attaches allocated sk object to the provided sock object.
> > If rfcomm_dlc_alloc() fails, we release the sk object, but leave the
> > dangling pointer in the sock object, which may cause use-after-free.
> >
> > Fix this by swapping calls to bt_sock_alloc() and rfcomm_dlc_alloc().
> >
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 5/9] net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:35 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:04 +0100
> > sock_init_data() attaches the allocated sk object to the provided sock
> > object. If ieee802154_create() fails later, the allocated sk object is
> > freed, but the dangling pointer remains in the provided sock object, which
> > may allow use-after-free.
> >
> > Clear the sk pointer in the sock object on error.
> >
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 6/9] net: inet: do not leave a dangling sk pointer in inet_create()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:37 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:05 +0100
> > sock_init_data() attaches the allocated sk object to the provided sock
> > object. If inet_create() fails later, the sk object is freed, but the
> > sock object retains the dangling pointer, which may create use-after-free
> > later.
> >
> > Clear the sk pointer in the sock object on error.
> >
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 8/9] net: warn, if pf->create does not clear sock->sk on error

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:40 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:07 +0100
> > All pf->create implementations have been fixed now to clear sock->sk on
> > error, when they deallocate the allocated sk object.
> >
> > Put a warning in place to make sure we don't break this promise in the
> > future.
> >
> > Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 9/9] Revert "net: do not leave a dangling sk pointer, when socket creation fails"

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:42 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:08 +0100
> > This reverts commit 6cd4a78d962bebbaf8beb7d2ead3f34120e3f7b2.
> >
> > inet/inet6->create() implementations have been fixed to explicitly NULL the
> > allocated sk object on error.
> >
> > A warning was put in place to make sure any future changes will not leave
> > a dangling pointer in pf->create() implementations.
> >
> > So this code is now redundant.
> >
> > Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 7/9] net: inet6: do not leave a dangling sk pointer in inet6_create()

[Eric Dumazet]

On Mon, Oct 14, 2024 at 11:39 PM Kuniyuki Iwashima <kuniyu@amazon.com> wrote:
>
> From: Ignat Korchagin <ignat@cloudflare.com>
> Date: Mon, 14 Oct 2024 16:38:06 +0100
> > sock_init_data() attaches the allocated sk pointer to the provided sock
> > object. If inet6_create() fails later, the sk object is released, but the
> > sock object retains the dangling sk pointer, which may cause use-after-free
> > later.
> >
> > Clear the sock sk pointer on error.
> >
> > Signed-off-by: Ignat Korchagin <ignat@cloudflare.com>
>
> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v3 0/9] do not leave dangling sk pointers in pf->create functions

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Oct 2024 16:37:59 +0100 you wrote:
> Some protocol family create() implementations have an error path after
> allocating the sk object and calling sock_init_data(). sock_init_data()
> attaches the allocated sk object to the sock object, provided by the
> caller.
> 
> If the create() implementation errors out after calling sock_init_data(),
> it releases the allocated sk object, but the caller ends up having a
> dangling sk pointer in its sock object on return. Subsequent manipulations
> on this sock object may try to access the sk pointer, because it is not
> NULL thus creating a use-after-free scenario.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()
    https://git.kernel.org/netdev/net-next/c/46f2a11cb82b
  - [net-next,v3,2/9] Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()
    https://git.kernel.org/netdev/net-next/c/7c4f78cdb8e7
  - [net-next,v3,3/9] Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()
    https://git.kernel.org/netdev/net-next/c/3945c799f12b
  - [net-next,v3,4/9] net: af_can: do not leave a dangling sk pointer in can_create()
    https://git.kernel.org/netdev/net-next/c/811a7ca7320c
  - [net-next,v3,5/9] net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()
    https://git.kernel.org/netdev/net-next/c/b4fcd63f6ef7
  - [net-next,v3,6/9] net: inet: do not leave a dangling sk pointer in inet_create()
    https://git.kernel.org/netdev/net-next/c/9365fa510c6f
  - [net-next,v3,7/9] net: inet6: do not leave a dangling sk pointer in inet6_create()
    https://git.kernel.org/netdev/net-next/c/9df99c395d0f
  - [net-next,v3,8/9] net: warn, if pf->create does not clear sock->sk on error
    https://git.kernel.org/netdev/net-next/c/48156296a08c
  - [net-next,v3,9/9] Revert "net: do not leave a dangling sk pointer, when socket creation fails"
    https://git.kernel.org/netdev/net-next/c/18429e6e0c2a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net-next v3 0/9] do not leave dangling sk pointers in pf->create functions

[patchwork-bot+bluetooth]

Hello:

This series was applied to bluetooth/bluetooth-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 14 Oct 2024 16:37:59 +0100 you wrote:
> Some protocol family create() implementations have an error path after
> allocating the sk object and calling sock_init_data(). sock_init_data()
> attaches the allocated sk object to the sock object, provided by the
> caller.
> 
> If the create() implementation errors out after calling sock_init_data(),
> it releases the allocated sk object, but the caller ends up having a
> dangling sk pointer in its sock object on return. Subsequent manipulations
> on this sock object may try to access the sk pointer, because it is not
> NULL thus creating a use-after-free scenario.
> 
> [...]

Here is the summary with links:
  - [net-next,v3,1/9] af_packet: avoid erroring out after sock_init_data() in packet_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/46f2a11cb82b
  - [net-next,v3,2/9] Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/7c4f78cdb8e7
  - [net-next,v3,3/9] Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()
    https://git.kernel.org/bluetooth/bluetooth-next/c/3945c799f12b
  - [net-next,v3,4/9] net: af_can: do not leave a dangling sk pointer in can_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/811a7ca7320c
  - [net-next,v3,5/9] net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/b4fcd63f6ef7
  - [net-next,v3,6/9] net: inet: do not leave a dangling sk pointer in inet_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/9365fa510c6f
  - [net-next,v3,7/9] net: inet6: do not leave a dangling sk pointer in inet6_create()
    https://git.kernel.org/bluetooth/bluetooth-next/c/9df99c395d0f
  - [net-next,v3,8/9] net: warn, if pf->create does not clear sock->sk on error
    https://git.kernel.org/bluetooth/bluetooth-next/c/48156296a08c
  - [net-next,v3,9/9] Revert "net: do not leave a dangling sk pointer, when socket creation fails"
    https://git.kernel.org/bluetooth/bluetooth-next/c/18429e6e0c2a

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html