[v2,0/8] do not leave dangling sk pointers in pf->create functions

Message

Ignat Korchagin Oct. 7, 2024, 9:34 p.m. UTC

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.

While the first patch in the series should be enough to handle this
scenario Eric Dumazet suggested that it would be a good idea to refactor
the code for the af_packet implementation to avoid the error path, which
leaves a dangling pointer, because it may be better for some tools like
kmemleak. I went a bit further and tried to actually fix all the
implementations, which could potentially leave a dangling sk pointer.

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 (8):
  net: explicitly clear the sk pointer, when pf->create fails
  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()
  inet6: do not leave a dangling sk pointer in inet6_create()

 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                |  7 ++++++-
 9 files changed, 46 insertions(+), 44 deletions(-)

Comments

Kuniyuki Iwashima Oct. 7, 2024, 9:57 p.m. UTC | #1

> [PATCH v2 0/8] do not leave dangling sk pointers in pf->create functions

For the future patches, please specify the target tree, net or net-next.


From: Ignat Korchagin <[e-mail]>
Date: Mon,  7 Oct 2024 22:34:54 +0100
> 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.
> 
> While the first patch in the series should be enough to handle this
> scenario Eric Dumazet suggested that it would be a good idea to refactor
> the code for the af_packet implementation to avoid the error path, which
> leaves a dangling pointer, because it may be better for some tools like
> kmemleak. I went a bit further and tried to actually fix all the
> implementations, which could potentially leave a dangling sk pointer.

I feel patch 2-8 are net-next materials as the first patch is enough
to fix the issue.

Also, once all protocols have moved sock_init_data() after the last
failure point, we can change the patch 1's part to

	err = pf->create(net, sock, protocol, kern);
	if (err) {
		DEBUG_NET_WARN_ON_ONCE(sock->sk);
		goto out_module_put;
	}

for the future protocols.