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; }
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;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;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!
Hello:
This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz luiz.von.dentz@intel.com:
On Wed, 18 Dec 2024 00:19:59 +0300 you 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.
[...]
Here is the summary with links: - Bluetooth: L2CAP: handle NULL sock pointer in l2cap_sock_alloc https://git.kernel.org/bluetooth/bluetooth-next/c/a5d2ee08adc1
You are awesome, thank you!
linux-stable-mirror@lists.linaro.org
-
Fedor Pchelkin -
Jakub Kicinski -
Kuniyuki Iwashima -
patchwork-bot+bluetooth＠kernel.org