A NULL sock pointer is passed into l2cap_sock_alloc() when it is called
from l2cap_sock_new_connection_cb() and the error handling paths should
also be aware of it.

Seemingly a more elegant solution would be to swap bt_sock_alloc() and
l2cap_chan_create() calls since they are not interdependent to that moment
but then l2cap_chan_create() adds the soon to be deallocated and still
dummy-initialized channel to the global list accessible by many L2CAP
paths. The channel would be removed from the list in short period of time
but be a bit more straight-forward here and just check for NULL instead of
changing the order of function calls.

Found by Linux Verification Center (linuxtesting.org) with SVACE static
analysis tool.

Fixes: 7c4f78cdb8e7 ("Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()")
Cc: stable@vger.kernel.org
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 net/bluetooth/l2cap_sock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
index 3d2553dcdb1b..49f97d4138ea 100644
--- a/net/bluetooth/l2cap_sock.c
+++ b/net/bluetooth/l2cap_sock.c
@@ -1888,7 +1888,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
 	chan = l2cap_chan_create();
 	if (!chan) {
 		sk_free(sk);
-		sock->sk = NULL;
+		if (sock)
+			sock->sk = NULL;
 		return NULL;
 	}
 
-- 
2.39.5

On Wed, 18. Dec 00:19, Fedor Pchelkin wrote:
> A NULL sock pointer is passed into l2cap_sock_alloc() when it is called
> from l2cap_sock_new_connection_cb() and the error handling paths should
> also be aware of it.
> 
> Seemingly a more elegant solution would be to swap bt_sock_alloc() and
> l2cap_chan_create() calls since they are not interdependent to that moment
> but then l2cap_chan_create() adds the soon to be deallocated and still
> dummy-initialized channel to the global list accessible by many L2CAP
> paths. The channel would be removed from the list in short period of time
> but be a bit more straight-forward here and just check for NULL instead of
> changing the order of function calls.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE static
> analysis tool.
> 
> Fixes: 7c4f78cdb8e7 ("Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---

Urgh.. a bit confused about which tree the patch should go to - net or
bluetooth.

I've now noticed the Fixes commit went directly via net-next as part of a
series (despite "Bluetooth: L2CAP:" patches usually go through bluetooth
tree first). So what about this patch?

>  net/bluetooth/l2cap_sock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 3d2553dcdb1b..49f97d4138ea 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1888,7 +1888,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
>  	chan = l2cap_chan_create();
>  	if (!chan) {
>  		sk_free(sk);
> -		sock->sk = NULL;
> +		if (sock)
> +			sock->sk = NULL;
>  		return NULL;
>  	}
>  
> -- 
> 2.39.5
>

On Thu, 9 Jan 2025 10:47:12 +0300 Fedor Pchelkin wrote:
> On Wed, 18. Dec 00:19, Fedor Pchelkin wrote:
> > A NULL sock pointer is passed into l2cap_sock_alloc() when it is called
> > from l2cap_sock_new_connection_cb() and the error handling paths should
> > also be aware of it.
> > 
> > Seemingly a more elegant solution would be to swap bt_sock_alloc() and
> > l2cap_chan_create() calls since they are not interdependent to that moment
> > but then l2cap_chan_create() adds the soon to be deallocated and still
> > dummy-initialized channel to the global list accessible by many L2CAP
> > paths. The channel would be removed from the list in short period of time
> > but be a bit more straight-forward here and just check for NULL instead of
> > changing the order of function calls.
> > 
> > Found by Linux Verification Center (linuxtesting.org) with SVACE static
> > analysis tool.
> > 
> > Fixes: 7c4f78cdb8e7 ("Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> > ---  
> 
> Urgh.. a bit confused about which tree the patch should go to - net or
> bluetooth.
> 
> I've now noticed the Fixes commit went directly via net-next as part of a
> series (despite "Bluetooth: L2CAP:" patches usually go through bluetooth
> tree first). So what about this patch?

7c4f78cdb8e7 went directly to net-next because it was a larger series touching
multiple sub-subsystems:

$ git log -12 --graph --oneline 2d859aff775df54
*   2d859aff775d Merge branch 'do-not-leave-dangling-sk-pointers-in-pf-create-functions'
|\  
| * 18429e6e0c2a Revert "net: do not leave a dangling sk pointer, when socket creation fails"
| * 48156296a08c net: warn, if pf->create does not clear sock->sk on error
| * 9df99c395d0f net: inet6: do not leave a dangling sk pointer in inet6_create()
| * 9365fa510c6f net: inet: do not leave a dangling sk pointer in inet_create()
| * b4fcd63f6ef7 net: ieee802154: do not leave a dangling sk pointer in ieee802154_create()
| * 811a7ca7320c net: af_can: do not leave a dangling sk pointer in can_create()
| * 3945c799f12b Bluetooth: RFCOMM: avoid leaving dangling sk pointer in rfcomm_sock_alloc()
| * 7c4f78cdb8e7 Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()
| * 46f2a11cb82b af_packet: avoid erroring out after sock_init_data() in packet_create()
|/  
* 397006ba5d91 net/sched: cbs: Fix integer overflow in cbs_set_port_rate()

On Thu, 09. Jan 07:11, Jakub Kicinski wrote:
> On Thu, 9 Jan 2025 10:47:12 +0300 Fedor Pchelkin wrote:
> > Urgh.. a bit confused about which tree the patch should go to - net or
> > bluetooth.
> > 
> > I've now noticed the Fixes commit went directly via net-next as part of a
> > series (despite "Bluetooth: L2CAP:" patches usually go through bluetooth
> > tree first). So what about this patch?
> 
> 7c4f78cdb8e7 went directly to net-next because it was a larger series touching
> multiple sub-subsystems:

Okay. So I'd expect Luiz to pick up the current patch then, thanks!

From: Fedor Pchelkin <pchelkin@ispras.ru>
Date: Wed, 18 Dec 2024 00:19:59 +0300
> A NULL sock pointer is passed into l2cap_sock_alloc() when it is called
> from l2cap_sock_new_connection_cb() and the error handling paths should
> also be aware of it.
> 
> Seemingly a more elegant solution would be to swap bt_sock_alloc() and
> l2cap_chan_create() calls since they are not interdependent to that moment
> but then l2cap_chan_create() adds the soon to be deallocated and still
> dummy-initialized channel to the global list accessible by many L2CAP
> paths. The channel would be removed from the list in short period of time
> but be a bit more straight-forward here and just check for NULL instead of
> changing the order of function calls.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE static
> analysis tool.
> 
> Fixes: 7c4f78cdb8e7 ("Bluetooth: L2CAP: do not leave dangling sk pointer on error in l2cap_sock_create()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

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


> ---
>  net/bluetooth/l2cap_sock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/l2cap_sock.c b/net/bluetooth/l2cap_sock.c
> index 3d2553dcdb1b..49f97d4138ea 100644
> --- a/net/bluetooth/l2cap_sock.c
> +++ b/net/bluetooth/l2cap_sock.c
> @@ -1888,7 +1888,8 @@ static struct sock *l2cap_sock_alloc(struct net *net, struct socket *sock,
>  	chan = l2cap_chan_create();
>  	if (!chan) {
>  		sk_free(sk);
> -		sock->sk = NULL;
> +		if (sock)
> +			sock->sk = NULL;
>  		return NULL;
>  	}
>  
> -- 
> 2.39.5