public inbox for netdev@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
@ 2026-03-09 15:59 Deepanshu Kartikey
  2026-03-10  3:18 ` Hillf Danton
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Deepanshu Kartikey @ 2026-03-09 15:59 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, horms
  Cc: mingo, tglx, jiayuan.chen, kees, netdev, dan.carpenter,
	linux-kernel, Deepanshu Kartikey, syzbot+f50072212ab792c86925

A race condition exists between lec_atm_close() setting priv->lecd
to NULL and concurrent access to priv->lecd in send_to_lecd(),
lec_handle_bridge(), and lec_atm_send(). When the socket is freed
via RCU while another thread is still using it, a use-after-free
occurs in sock_def_readable() when accessing the socket's wait queue.

The root cause is that lec_atm_close() clears priv->lecd without
any synchronization, while callers dereference priv->lecd without
any protection against concurrent teardown.

Fix this by converting priv->lecd to an RCU-protected pointer:
- Mark priv->lecd as __rcu in lec.h
- Use rcu_assign_pointer() in lec_atm_close() and lecd_attach()
  for safe pointer assignment
- Use rcu_access_pointer() for NULL checks that do not dereference
  the pointer in lec_start_xmit(), lec_push(), send_to_lecd() and
  lecd_attach()
- Use rcu_read_lock/rcu_dereference/rcu_read_unlock in send_to_lecd(),
  lec_handle_bridge() and lec_atm_send() to safely access lecd
- Use rcu_assign_pointer() followed by synchronize_rcu() in
  lec_atm_close() to ensure all readers have completed before
  proceeding. This is safe since lec_atm_close() is called from
  vcc_release() which holds lock_sock(), a sleeping lock.
- Remove the manual sk_receive_queue drain from lec_atm_close()
  since vcc_destroy_socket() already drains it after lec_atm_close()
  returns.

v2: Switch from spinlock + sock_hold/put approach to RCU to properly
    fix the race. The v1 spinlock approach had two issues pointed out
    by Eric Dumazet:
    1. priv->lecd was still accessed directly after releasing the
       lock instead of using a local copy.
    2. The spinlock did not prevent packets being queued after
       lec_atm_close() drains sk_receive_queue since timer and
       workqueue paths bypass netif_stop_queue().

Note: Syzbot patch testing was attempted but the test VM terminated
    unexpectedly with "Connection to localhost closed by remote host",
    likely due to a QEMU AHCI emulation issue unrelated to this fix.
    Compile testing with "make W=1 net/atm/lec.o" passes cleanly.

Reported-by: syzbot+f50072212ab792c86925@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=f50072212ab792c86925
Link: https://lore.kernel.org/all/20260309093614.502094-1-kartikey406@gmail.com/T/ [v1]
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 net/atm/lec.c | 70 +++++++++++++++++++++++++++++++++------------------
 net/atm/lec.h |  2 +-
 2 files changed, 46 insertions(+), 26 deletions(-)

diff --git a/net/atm/lec.c b/net/atm/lec.c
index fb93c6e1c329..32adf28663a0 100644
--- a/net/atm/lec.c
+++ b/net/atm/lec.c
@@ -154,10 +154,19 @@ static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev)
 					/* 0x01 is topology change */
 
 		priv = netdev_priv(dev);
-		atm_force_charge(priv->lecd, skb2->truesize);
-		sk = sk_atm(priv->lecd);
-		skb_queue_tail(&sk->sk_receive_queue, skb2);
-		sk->sk_data_ready(sk);
+		struct atm_vcc *vcc;
+
+		rcu_read_lock();
+		vcc = rcu_dereference(priv->lecd);
+		if (vcc) {
+			atm_force_charge(vcc, skb2->truesize);
+			sk = sk_atm(vcc);
+			skb_queue_tail(&sk->sk_receive_queue, skb2);
+			sk->sk_data_ready(sk);
+		} else {
+			dev_kfree_skb(skb2);
+		}
+		rcu_read_unlock();
 	}
 }
 #endif /* IS_ENABLED(CONFIG_BRIDGE) */
@@ -216,7 +225,7 @@ static netdev_tx_t lec_start_xmit(struct sk_buff *skb,
 	int is_rdesc;
 
 	pr_debug("called\n");
-	if (!priv->lecd) {
+	if (!rcu_access_pointer(priv->lecd)) {
 		pr_info("%s:No lecd attached\n", dev->name);
 		dev->stats.tx_errors++;
 		netif_stop_queue(dev);
@@ -449,10 +459,19 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
 				break;
 			skb2->len = sizeof(struct atmlec_msg);
 			skb_copy_to_linear_data(skb2, mesg, sizeof(*mesg));
-			atm_force_charge(priv->lecd, skb2->truesize);
-			sk = sk_atm(priv->lecd);
-			skb_queue_tail(&sk->sk_receive_queue, skb2);
-			sk->sk_data_ready(sk);
+			struct atm_vcc *vcc;
+
+			rcu_read_lock();
+			vcc = rcu_dereference(priv->lecd);
+			if (vcc) {
+				atm_force_charge(vcc, skb2->truesize);
+				sk = sk_atm(vcc);
+				skb_queue_tail(&sk->sk_receive_queue, skb2);
+				sk->sk_data_ready(sk);
+			} else {
+				dev_kfree_skb(skb2);
+			}
+			rcu_read_unlock();
 		}
 	}
 #endif /* IS_ENABLED(CONFIG_BRIDGE) */
@@ -468,23 +486,16 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
 
 static void lec_atm_close(struct atm_vcc *vcc)
 {
-	struct sk_buff *skb;
 	struct net_device *dev = (struct net_device *)vcc->proto_data;
 	struct lec_priv *priv = netdev_priv(dev);
 
-	priv->lecd = NULL;
+	rcu_assign_pointer(priv->lecd, NULL);
+	synchronize_rcu();
 	/* Do something needful? */
 
 	netif_stop_queue(dev);
 	lec_arp_destroy(priv);
 
-	if (skb_peek(&sk_atm(vcc)->sk_receive_queue))
-		pr_info("%s closing with messages pending\n", dev->name);
-	while ((skb = skb_dequeue(&sk_atm(vcc)->sk_receive_queue))) {
-		atm_return(vcc, skb->truesize);
-		dev_kfree_skb(skb);
-	}
-
 	pr_info("%s: Shut down!\n", dev->name);
 	module_put(THIS_MODULE);
 }
@@ -510,12 +521,14 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
 	     const unsigned char *mac_addr, const unsigned char *atm_addr,
 	     struct sk_buff *data)
 {
+	struct atm_vcc *vcc;
 	struct sock *sk;
 	struct sk_buff *skb;
 	struct atmlec_msg *mesg;
 
-	if (!priv || !priv->lecd)
+	if (!priv || !rcu_access_pointer(priv->lecd))
 		return -1;
+
 	skb = alloc_skb(sizeof(struct atmlec_msg), GFP_ATOMIC);
 	if (!skb)
 		return -1;
@@ -532,18 +545,27 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
 	if (atm_addr)
 		memcpy(&mesg->content.normal.atm_addr, atm_addr, ATM_ESA_LEN);
 
-	atm_force_charge(priv->lecd, skb->truesize);
-	sk = sk_atm(priv->lecd);
+	rcu_read_lock();
+	vcc = rcu_dereference(priv->lecd);
+	if (!vcc) {
+		rcu_read_unlock();
+		kfree_skb(skb);
+		return -1;
+	}
+
+	atm_force_charge(vcc, skb->truesize);
+	sk = sk_atm(vcc);
 	skb_queue_tail(&sk->sk_receive_queue, skb);
 	sk->sk_data_ready(sk);
 
 	if (data != NULL) {
 		pr_debug("about to send %d bytes of data\n", data->len);
-		atm_force_charge(priv->lecd, data->truesize);
+		atm_force_charge(vcc, data->truesize);
 		skb_queue_tail(&sk->sk_receive_queue, data);
 		sk->sk_data_ready(sk);
 	}
 
+	rcu_read_unlock();
 	return 0;
 }
 
@@ -618,7 +640,7 @@ static void lec_push(struct atm_vcc *vcc, struct sk_buff *skb)
 
 		atm_return(vcc, skb->truesize);
 		if (*(__be16 *) skb->data == htons(priv->lecid) ||
-		    !priv->lecd || !(dev->flags & IFF_UP)) {
+		    !rcu_access_pointer(priv->lecd) || !(dev->flags & IFF_UP)) {
 			/*
 			 * Probably looping back, or if lecd is missing,
 			 * lecd has gone down
@@ -753,12 +775,12 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
 		priv = netdev_priv(dev_lec[i]);
 	} else {
 		priv = netdev_priv(dev_lec[i]);
-		if (priv->lecd)
+		if (rcu_access_pointer(priv->lecd))
 			return -EADDRINUSE;
 	}
 	lec_arp_init(priv);
 	priv->itfnum = i;	/* LANE2 addition */
-	priv->lecd = vcc;
+	rcu_assign_pointer(priv->lecd, vcc);
 	vcc->dev = &lecatm_dev;
 	vcc_insert_socket(sk_atm(vcc));
 
diff --git a/net/atm/lec.h b/net/atm/lec.h
index be0e2667bd8c..ec85709bf818 100644
--- a/net/atm/lec.h
+++ b/net/atm/lec.h
@@ -91,7 +91,7 @@ struct lec_priv {
 						 */
 	spinlock_t lec_arp_lock;
 	struct atm_vcc *mcast_vcc;		/* Default Multicast Send VCC */
-	struct atm_vcc *lecd;
+	struct atm_vcc __rcu *lecd;
 	struct delayed_work lec_arp_work;	/* C10 */
 	unsigned int maximum_unknown_frame_count;
 						/*
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-09 15:59 [PATCH v2] atm: lec: fix use-after-free in sock_def_readable() Deepanshu Kartikey
@ 2026-03-10  3:18 ` Hillf Danton
  2026-03-12  1:03   ` Deepanshu Kartikey
  2026-03-14 12:00 ` Deepanshu Kartikey
  2026-03-14 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Hillf Danton @ 2026-03-10  3:18 UTC (permalink / raw)
  To: Deepanshu Kartikey
  Cc: edumazet, netdev, linux-kernel, syzbot+f50072212ab792c86925

On Mon,  9 Mar 2026 21:29:08 +0530 Deepanshu Kartikey wrote:
> A race condition exists between lec_atm_close() setting priv->lecd
> to NULL and concurrent access to priv->lecd in send_to_lecd(),
> lec_handle_bridge(), and lec_atm_send(). When the socket is freed
> via RCU while another thread is still using it, a use-after-free
> occurs in sock_def_readable() when accessing the socket's wait queue.
> 
> The root cause is that lec_atm_close() clears priv->lecd without
> any synchronization, while callers dereference priv->lecd without
> any protection against concurrent teardown.
> 
> Fix this by converting priv->lecd to an RCU-protected pointer:
> - Mark priv->lecd as __rcu in lec.h
> - Use rcu_assign_pointer() in lec_atm_close() and lecd_attach()
>   for safe pointer assignment
> - Use rcu_access_pointer() for NULL checks that do not dereference
>   the pointer in lec_start_xmit(), lec_push(), send_to_lecd() and
>   lecd_attach()
> - Use rcu_read_lock/rcu_dereference/rcu_read_unlock in send_to_lecd(),
>   lec_handle_bridge() and lec_atm_send() to safely access lecd
> - Use rcu_assign_pointer() followed by synchronize_rcu() in
>   lec_atm_close() to ensure all readers have completed before
>   proceeding. This is safe since lec_atm_close() is called from
>   vcc_release() which holds lock_sock(), a sleeping lock.
> - Remove the manual sk_receive_queue drain from lec_atm_close()
>   since vcc_destroy_socket() already drains it after lec_atm_close()
>   returns.
> 
> v2: Switch from spinlock + sock_hold/put approach to RCU to properly
>     fix the race. The v1 spinlock approach had two issues pointed out
>     by Eric Dumazet:
>     1. priv->lecd was still accessed directly after releasing the
>        lock instead of using a local copy.
>     2. The spinlock did not prevent packets being queued after
>        lec_atm_close() drains sk_receive_queue since timer and
>        workqueue paths bypass netif_stop_queue().
> 
> Note: Syzbot patch testing was attempted but the test VM terminated
>     unexpectedly with "Connection to localhost closed by remote host",
>     likely due to a QEMU AHCI emulation issue unrelated to this fix.
>     Compile testing with "make W=1 net/atm/lec.o" passes cleanly.
> 
> Reported-by: syzbot+f50072212ab792c86925@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f50072212ab792c86925
> Link: https://lore.kernel.org/all/20260309093614.502094-1-kartikey406@gmail.com/T/ [v1]
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  net/atm/lec.c | 70 +++++++++++++++++++++++++++++++++------------------
>  net/atm/lec.h |  2 +-
>  2 files changed, 46 insertions(+), 26 deletions(-)
> 
> diff --git a/net/atm/lec.c b/net/atm/lec.c
> index fb93c6e1c329..32adf28663a0 100644
> --- a/net/atm/lec.c
> +++ b/net/atm/lec.c
> @@ -154,10 +154,19 @@ static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev)
>  					/* 0x01 is topology change */
>  
>  		priv = netdev_priv(dev);
> -		atm_force_charge(priv->lecd, skb2->truesize);
> -		sk = sk_atm(priv->lecd);
> -		skb_queue_tail(&sk->sk_receive_queue, skb2);
> -		sk->sk_data_ready(sk);
> +		struct atm_vcc *vcc;
> +
> +		rcu_read_lock();
> +		vcc = rcu_dereference(priv->lecd);
> +		if (vcc) {
> +			atm_force_charge(vcc, skb2->truesize);
> +			sk = sk_atm(vcc);
> +			skb_queue_tail(&sk->sk_receive_queue, skb2);
> +			sk->sk_data_ready(sk);
> +		} else {
> +			dev_kfree_skb(skb2);
> +		}
> +		rcu_read_unlock();
>  	}
>  }
>  #endif /* IS_ENABLED(CONFIG_BRIDGE) */
> @@ -216,7 +225,7 @@ static netdev_tx_t lec_start_xmit(struct sk_buff *skb,
>  	int is_rdesc;
>  
>  	pr_debug("called\n");
> -	if (!priv->lecd) {
> +	if (!rcu_access_pointer(priv->lecd)) {
>  		pr_info("%s:No lecd attached\n", dev->name);
>  		dev->stats.tx_errors++;
>  		netif_stop_queue(dev);
> @@ -449,10 +459,19 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
>  				break;
>  			skb2->len = sizeof(struct atmlec_msg);
>  			skb_copy_to_linear_data(skb2, mesg, sizeof(*mesg));
> -			atm_force_charge(priv->lecd, skb2->truesize);
> -			sk = sk_atm(priv->lecd);
> -			skb_queue_tail(&sk->sk_receive_queue, skb2);
> -			sk->sk_data_ready(sk);
> +			struct atm_vcc *vcc;
> +
> +			rcu_read_lock();
> +			vcc = rcu_dereference(priv->lecd);
> +			if (vcc) {
> +				atm_force_charge(vcc, skb2->truesize);
> +				sk = sk_atm(vcc);
> +				skb_queue_tail(&sk->sk_receive_queue, skb2);
> +				sk->sk_data_ready(sk);
> +			} else {
> +				dev_kfree_skb(skb2);
> +			}
> +			rcu_read_unlock();
>  		}
>  	}
>  #endif /* IS_ENABLED(CONFIG_BRIDGE) */
> @@ -468,23 +486,16 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
>  
>  static void lec_atm_close(struct atm_vcc *vcc)
>  {
> -	struct sk_buff *skb;
>  	struct net_device *dev = (struct net_device *)vcc->proto_data;
>  	struct lec_priv *priv = netdev_priv(dev);
>  
> -	priv->lecd = NULL;
> +	rcu_assign_pointer(priv->lecd, NULL);
> +	synchronize_rcu();
>  	/* Do something needful? */
>  
At this point priv->lecd is no longer used, so why not make lecd valid
throughout the lifespan of priv and free it after stopping dev queue,
instead of the tedious rcu trick?

>  	netif_stop_queue(dev);
>  	lec_arp_destroy(priv);
>  
> -	if (skb_peek(&sk_atm(vcc)->sk_receive_queue))
> -		pr_info("%s closing with messages pending\n", dev->name);
> -	while ((skb = skb_dequeue(&sk_atm(vcc)->sk_receive_queue))) {
> -		atm_return(vcc, skb->truesize);
> -		dev_kfree_skb(skb);
> -	}
> -
>  	pr_info("%s: Shut down!\n", dev->name);
>  	module_put(THIS_MODULE);
>  }
> @@ -510,12 +521,14 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
>  	     const unsigned char *mac_addr, const unsigned char *atm_addr,
>  	     struct sk_buff *data)
>  {
> +	struct atm_vcc *vcc;
>  	struct sock *sk;
>  	struct sk_buff *skb;
>  	struct atmlec_msg *mesg;
>  
> -	if (!priv || !priv->lecd)
> +	if (!priv || !rcu_access_pointer(priv->lecd))
>  		return -1;
> +
>  	skb = alloc_skb(sizeof(struct atmlec_msg), GFP_ATOMIC);
>  	if (!skb)
>  		return -1;
> @@ -532,18 +545,27 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
>  	if (atm_addr)
>  		memcpy(&mesg->content.normal.atm_addr, atm_addr, ATM_ESA_LEN);
>  
> -	atm_force_charge(priv->lecd, skb->truesize);
> -	sk = sk_atm(priv->lecd);
> +	rcu_read_lock();
> +	vcc = rcu_dereference(priv->lecd);
> +	if (!vcc) {
> +		rcu_read_unlock();
> +		kfree_skb(skb);
> +		return -1;
> +	}
> +
> +	atm_force_charge(vcc, skb->truesize);
> +	sk = sk_atm(vcc);
>  	skb_queue_tail(&sk->sk_receive_queue, skb);
>  	sk->sk_data_ready(sk);
>  
>  	if (data != NULL) {
>  		pr_debug("about to send %d bytes of data\n", data->len);
> -		atm_force_charge(priv->lecd, data->truesize);
> +		atm_force_charge(vcc, data->truesize);
>  		skb_queue_tail(&sk->sk_receive_queue, data);
>  		sk->sk_data_ready(sk);
>  	}
>  
> +	rcu_read_unlock();
>  	return 0;
>  }
>  
> @@ -618,7 +640,7 @@ static void lec_push(struct atm_vcc *vcc, struct sk_buff *skb)
>  
>  		atm_return(vcc, skb->truesize);
>  		if (*(__be16 *) skb->data == htons(priv->lecid) ||
> -		    !priv->lecd || !(dev->flags & IFF_UP)) {
> +		    !rcu_access_pointer(priv->lecd) || !(dev->flags & IFF_UP)) {
>  			/*
>  			 * Probably looping back, or if lecd is missing,
>  			 * lecd has gone down
> @@ -753,12 +775,12 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
>  		priv = netdev_priv(dev_lec[i]);
>  	} else {
>  		priv = netdev_priv(dev_lec[i]);
> -		if (priv->lecd)
> +		if (rcu_access_pointer(priv->lecd))
>  			return -EADDRINUSE;
>  	}
>  	lec_arp_init(priv);
>  	priv->itfnum = i;	/* LANE2 addition */
> -	priv->lecd = vcc;
> +	rcu_assign_pointer(priv->lecd, vcc);
>  	vcc->dev = &lecatm_dev;
>  	vcc_insert_socket(sk_atm(vcc));
>  
> diff --git a/net/atm/lec.h b/net/atm/lec.h
> index be0e2667bd8c..ec85709bf818 100644
> --- a/net/atm/lec.h
> +++ b/net/atm/lec.h
> @@ -91,7 +91,7 @@ struct lec_priv {
>  						 */
>  	spinlock_t lec_arp_lock;
>  	struct atm_vcc *mcast_vcc;		/* Default Multicast Send VCC */
> -	struct atm_vcc *lecd;
> +	struct atm_vcc __rcu *lecd;
>  	struct delayed_work lec_arp_work;	/* C10 */
>  	unsigned int maximum_unknown_frame_count;
>  						/*
> -- 
> 2.43.0

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-10  3:18 ` Hillf Danton
@ 2026-03-12  1:03   ` Deepanshu Kartikey
  2026-03-14  2:53     ` Hillf Danton
  0 siblings, 1 reply; 7+ messages in thread
From: Deepanshu Kartikey @ 2026-03-12  1:03 UTC (permalink / raw)
  To: Hillf Danton; +Cc: edumazet, netdev, linux-kernel, syzbot+f50072212ab792c86925

On Tue, Mar 10, 2026 at 8:48 AM Hillf Danton <hdanton@sina.com> wrote:
>
> At this point priv->lecd is no longer used, so why not make lecd valid
> throughout the lifespan of priv and free it after stopping dev queue,
> instead of the tedious rcu trick?
>

Thank you for the suggestion.

I investigated this approach. While netif_stop_queue() stops
the TX path and cancel_delayed_work_sync() in lec_arp_destroy()
stops lec_arp_work, the bug is actually triggered from
mld_ifc_work (IPv6 multicast workqueue) which calls:

mld_ifc_work -> mld_sendpack -> ip6_output
    -> lec_start_xmit -> lec_arp_resolve -> send_to_lecd

This workqueue belongs to the IPv6 multicast subsystem and
is completely outside ATM/LEC control. Neither
netif_stop_queue() nor lec_arp_destroy() can stop it, so
simply reordering the calls in lec_atm_close() would not
fix the race.

The RCU approach with synchronize_rcu() ensures ALL callers
including mld_ifc_work have finished before priv->lecd is
cleared.

Deepanshu Kartikey

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-12  1:03   ` Deepanshu Kartikey
@ 2026-03-14  2:53     ` Hillf Danton
  0 siblings, 0 replies; 7+ messages in thread
From: Hillf Danton @ 2026-03-14  2:53 UTC (permalink / raw)
  To: Deepanshu Kartikey
  Cc: edumazet, netdev, linux-kernel, syzbot+f50072212ab792c86925

On Thu, 12 Mar 2026 06:33:53 +0530 Deepanshu Kartikey wrote:
> On Tue, Mar 10, 2026 at 8:48 AM Hillf Danton <hdanton@sina.com> wrote:
> >
> > At this point priv->lecd is no longer used, so why not make lecd valid
> > throughout the lifespan of priv and free it after stopping dev queue,
> > instead of the tedious rcu trick?
> >
> 
> Thank you for the suggestion.
> 
> I investigated this approach. While netif_stop_queue() stops
> the TX path and cancel_delayed_work_sync() in lec_arp_destroy()
> stops lec_arp_work, the bug is actually triggered from
> mld_ifc_work (IPv6 multicast workqueue) which calls:
> 
> mld_ifc_work -> mld_sendpack -> ip6_output
>     -> lec_start_xmit -> lec_arp_resolve -> send_to_lecd
> 
> This workqueue belongs to the IPv6 multicast subsystem and
> is completely outside ATM/LEC control. Neither
> netif_stop_queue() nor lec_arp_destroy() can stop it, so
> simply reordering the calls in lec_atm_close() would not
> fix the race.
> 
> The RCU approach with synchronize_rcu() ensures ALL callers
> including mld_ifc_work have finished before priv->lecd is
> cleared.
>
After another look the uaf [1] is due to the race

	vcc_release()		mld_sendpack()
	---			---
				ip6_output()
				__dev_queue_xmit()
				  rcu_read_lock_bh();  // rcu section
				  __dev_xmit_skb()
				  netdev_start_xmit()
				  lec_start_xmit()
				    if (!priv->lecd) {  // check lecd
					kfree_skb(skb);
					return NETDEV_TX_OK;
				    }

	vcc_destroy_socket()
	vcc->dev->ops->close(vcc);
	lec_atm_close()
	  priv->lecd = NULL;	// clear lecd
	  netif_stop_queue(dev);
	sock_put() // free sock

				  lec_arp_resolve()
				  send_to_lecd()
				  sock_def_readable() // uaf

and syncing rcu after clearing lecd is the correct fix because lecd is checked
with rcu lock held.

[1] Subject: [syzbot] [net?] KASAN: slab-use-after-free Read in sock_def_readable (2)
https://lore.kernel.org/all/69ad7ccb.a00a0220.b130.0003.GAE@google.com/

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-09 15:59 [PATCH v2] atm: lec: fix use-after-free in sock_def_readable() Deepanshu Kartikey
  2026-03-10  3:18 ` Hillf Danton
@ 2026-03-14 12:00 ` Deepanshu Kartikey
  2026-03-14 12:10   ` Eric Dumazet
  2026-03-14 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Deepanshu Kartikey @ 2026-03-14 12:00 UTC (permalink / raw)
  To: davem, edumazet, kuba, pabeni, horms
  Cc: mingo, tglx, jiayuan.chen, kees, netdev, dan.carpenter,
	linux-kernel, syzbot+f50072212ab792c86925

On Mon, Mar 9, 2026 at 9:29 PM Deepanshu Kartikey <kartikey406@gmail.com> wrote:
>
> A race condition exists between lec_atm_close() setting priv->lecd
> to NULL and concurrent access to priv->lecd in send_to_lecd(),
> lec_handle_bridge(), and lec_atm_send(). When the socket is freed
> via RCU while another thread is still using it, a use-after-free
> occurs in sock_def_readable() when accessing the socket's wait queue.
>
> The root cause is that lec_atm_close() clears priv->lecd without
> any synchronization, while callers dereference priv->lecd without
> any protection against concurrent teardown.
>
> Fix this by converting priv->lecd to an RCU-protected pointer:
> - Mark priv->lecd as __rcu in lec.h
> - Use rcu_assign_pointer() in lec_atm_close() and lecd_attach()
>   for safe pointer assignment
> - Use rcu_access_pointer() for NULL checks that do not dereference
>   the pointer in lec_start_xmit(), lec_push(), send_to_lecd() and
>   lecd_attach()
> - Use rcu_read_lock/rcu_dereference/rcu_read_unlock in send_to_lecd(),
>   lec_handle_bridge() and lec_atm_send() to safely access lecd
> - Use rcu_assign_pointer() followed by synchronize_rcu() in
>   lec_atm_close() to ensure all readers have completed before
>   proceeding. This is safe since lec_atm_close() is called from
>   vcc_release() which holds lock_sock(), a sleeping lock.
> - Remove the manual sk_receive_queue drain from lec_atm_close()
>   since vcc_destroy_socket() already drains it after lec_atm_close()
>   returns.
>
> v2: Switch from spinlock + sock_hold/put approach to RCU to properly
>     fix the race. The v1 spinlock approach had two issues pointed out
>     by Eric Dumazet:
>     1. priv->lecd was still accessed directly after releasing the
>        lock instead of using a local copy.
>     2. The spinlock did not prevent packets being queued after
>        lec_atm_close() drains sk_receive_queue since timer and
>        workqueue paths bypass netif_stop_queue().
>
> Note: Syzbot patch testing was attempted but the test VM terminated
>     unexpectedly with "Connection to localhost closed by remote host",
>     likely due to a QEMU AHCI emulation issue unrelated to this fix.
>     Compile testing with "make W=1 net/atm/lec.o" passes cleanly.
>
> Reported-by: syzbot+f50072212ab792c86925@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=f50072212ab792c86925
> Link: https://lore.kernel.org/all/20260309093614.502094-1-kartikey406@gmail.com/T/ [v1]
> Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
> ---
>  net/atm/lec.c | 70 +++++++++++++++++++++++++++++++++------------------
>  net/atm/lec.h |  2 +-
>  2 files changed, 46 insertions(+), 26 deletions(-)
>
> diff --git a/net/atm/lec.c b/net/atm/lec.c
> index fb93c6e1c329..32adf28663a0 100644
> --- a/net/atm/lec.c
> +++ b/net/atm/lec.c
> @@ -154,10 +154,19 @@ static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev)
>                                         /* 0x01 is topology change */
>
>                 priv = netdev_priv(dev);
> -               atm_force_charge(priv->lecd, skb2->truesize);
> -               sk = sk_atm(priv->lecd);
> -               skb_queue_tail(&sk->sk_receive_queue, skb2);
> -               sk->sk_data_ready(sk);
> +               struct atm_vcc *vcc;
> +
> +               rcu_read_lock();
> +               vcc = rcu_dereference(priv->lecd);
> +               if (vcc) {
> +                       atm_force_charge(vcc, skb2->truesize);
> +                       sk = sk_atm(vcc);
> +                       skb_queue_tail(&sk->sk_receive_queue, skb2);
> +                       sk->sk_data_ready(sk);
> +               } else {
> +                       dev_kfree_skb(skb2);
> +               }
> +               rcu_read_unlock();
>         }
>  }
>  #endif /* IS_ENABLED(CONFIG_BRIDGE) */
> @@ -216,7 +225,7 @@ static netdev_tx_t lec_start_xmit(struct sk_buff *skb,
>         int is_rdesc;
>
>         pr_debug("called\n");
> -       if (!priv->lecd) {
> +       if (!rcu_access_pointer(priv->lecd)) {
>                 pr_info("%s:No lecd attached\n", dev->name);
>                 dev->stats.tx_errors++;
>                 netif_stop_queue(dev);
> @@ -449,10 +459,19 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
>                                 break;
>                         skb2->len = sizeof(struct atmlec_msg);
>                         skb_copy_to_linear_data(skb2, mesg, sizeof(*mesg));
> -                       atm_force_charge(priv->lecd, skb2->truesize);
> -                       sk = sk_atm(priv->lecd);
> -                       skb_queue_tail(&sk->sk_receive_queue, skb2);
> -                       sk->sk_data_ready(sk);
> +                       struct atm_vcc *vcc;
> +
> +                       rcu_read_lock();
> +                       vcc = rcu_dereference(priv->lecd);
> +                       if (vcc) {
> +                               atm_force_charge(vcc, skb2->truesize);
> +                               sk = sk_atm(vcc);
> +                               skb_queue_tail(&sk->sk_receive_queue, skb2);
> +                               sk->sk_data_ready(sk);
> +                       } else {
> +                               dev_kfree_skb(skb2);
> +                       }
> +                       rcu_read_unlock();
>                 }
>         }
>  #endif /* IS_ENABLED(CONFIG_BRIDGE) */
> @@ -468,23 +486,16 @@ static int lec_atm_send(struct atm_vcc *vcc, struct sk_buff *skb)
>
>  static void lec_atm_close(struct atm_vcc *vcc)
>  {
> -       struct sk_buff *skb;
>         struct net_device *dev = (struct net_device *)vcc->proto_data;
>         struct lec_priv *priv = netdev_priv(dev);
>
> -       priv->lecd = NULL;
> +       rcu_assign_pointer(priv->lecd, NULL);
> +       synchronize_rcu();
>         /* Do something needful? */
>
>         netif_stop_queue(dev);
>         lec_arp_destroy(priv);
>
> -       if (skb_peek(&sk_atm(vcc)->sk_receive_queue))
> -               pr_info("%s closing with messages pending\n", dev->name);
> -       while ((skb = skb_dequeue(&sk_atm(vcc)->sk_receive_queue))) {
> -               atm_return(vcc, skb->truesize);
> -               dev_kfree_skb(skb);
> -       }
> -
>         pr_info("%s: Shut down!\n", dev->name);
>         module_put(THIS_MODULE);
>  }
> @@ -510,12 +521,14 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
>              const unsigned char *mac_addr, const unsigned char *atm_addr,
>              struct sk_buff *data)
>  {
> +       struct atm_vcc *vcc;
>         struct sock *sk;
>         struct sk_buff *skb;
>         struct atmlec_msg *mesg;
>
> -       if (!priv || !priv->lecd)
> +       if (!priv || !rcu_access_pointer(priv->lecd))
>                 return -1;
> +
>         skb = alloc_skb(sizeof(struct atmlec_msg), GFP_ATOMIC);
>         if (!skb)
>                 return -1;
> @@ -532,18 +545,27 @@ send_to_lecd(struct lec_priv *priv, atmlec_msg_type type,
>         if (atm_addr)
>                 memcpy(&mesg->content.normal.atm_addr, atm_addr, ATM_ESA_LEN);
>
> -       atm_force_charge(priv->lecd, skb->truesize);
> -       sk = sk_atm(priv->lecd);
> +       rcu_read_lock();
> +       vcc = rcu_dereference(priv->lecd);
> +       if (!vcc) {
> +               rcu_read_unlock();
> +               kfree_skb(skb);
> +               return -1;
> +       }
> +
> +       atm_force_charge(vcc, skb->truesize);
> +       sk = sk_atm(vcc);
>         skb_queue_tail(&sk->sk_receive_queue, skb);
>         sk->sk_data_ready(sk);
>
>         if (data != NULL) {
>                 pr_debug("about to send %d bytes of data\n", data->len);
> -               atm_force_charge(priv->lecd, data->truesize);
> +               atm_force_charge(vcc, data->truesize);
>                 skb_queue_tail(&sk->sk_receive_queue, data);
>                 sk->sk_data_ready(sk);
>         }
>
> +       rcu_read_unlock();
>         return 0;
>  }
>
> @@ -618,7 +640,7 @@ static void lec_push(struct atm_vcc *vcc, struct sk_buff *skb)
>
>                 atm_return(vcc, skb->truesize);
>                 if (*(__be16 *) skb->data == htons(priv->lecid) ||
> -                   !priv->lecd || !(dev->flags & IFF_UP)) {
> +                   !rcu_access_pointer(priv->lecd) || !(dev->flags & IFF_UP)) {
>                         /*
>                          * Probably looping back, or if lecd is missing,
>                          * lecd has gone down
> @@ -753,12 +775,12 @@ static int lecd_attach(struct atm_vcc *vcc, int arg)
>                 priv = netdev_priv(dev_lec[i]);
>         } else {
>                 priv = netdev_priv(dev_lec[i]);
> -               if (priv->lecd)
> +               if (rcu_access_pointer(priv->lecd))
>                         return -EADDRINUSE;
>         }
>         lec_arp_init(priv);
>         priv->itfnum = i;       /* LANE2 addition */
> -       priv->lecd = vcc;
> +       rcu_assign_pointer(priv->lecd, vcc);
>         vcc->dev = &lecatm_dev;
>         vcc_insert_socket(sk_atm(vcc));
>
> diff --git a/net/atm/lec.h b/net/atm/lec.h
> index be0e2667bd8c..ec85709bf818 100644
> --- a/net/atm/lec.h
> +++ b/net/atm/lec.h
> @@ -91,7 +91,7 @@ struct lec_priv {
>                                                  */
>         spinlock_t lec_arp_lock;
>         struct atm_vcc *mcast_vcc;              /* Default Multicast Send VCC */
> -       struct atm_vcc *lecd;
> +       struct atm_vcc __rcu *lecd;
>         struct delayed_work lec_arp_work;       /* C10 */
>         unsigned int maximum_unknown_frame_count;
>                                                 /*
> --
> 2.43.0
>

Hi Eric,

Gentle ping on the v2 patch below.
Please let me know if you have any concerns.

Thanks,
Deepanshu Kartikey

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-14 12:00 ` Deepanshu Kartikey
@ 2026-03-14 12:10   ` Eric Dumazet
  0 siblings, 0 replies; 7+ messages in thread
From: Eric Dumazet @ 2026-03-14 12:10 UTC (permalink / raw)
  To: Deepanshu Kartikey
  Cc: davem, kuba, pabeni, horms, mingo, tglx, jiayuan.chen, kees,
	netdev, dan.carpenter, linux-kernel, syzbot+f50072212ab792c86925

On Sat, Mar 14, 2026 at 1:00 PM Deepanshu Kartikey
<kartikey406@gmail.com> wrote:
>
> Hi Eric,
>
> Gentle ping on the v2 patch below.
> Please let me know if you have any concerns.
>
> Thanks,
> Deepanshu Kartikey

Patch LGTM, thanks.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()
  2026-03-09 15:59 [PATCH v2] atm: lec: fix use-after-free in sock_def_readable() Deepanshu Kartikey
  2026-03-10  3:18 ` Hillf Danton
  2026-03-14 12:00 ` Deepanshu Kartikey
@ 2026-03-14 15:40 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-03-14 15:40 UTC (permalink / raw)
  To: Deepanshu Kartikey
  Cc: davem, edumazet, kuba, pabeni, horms, mingo, tglx, jiayuan.chen,
	kees, netdev, dan.carpenter, linux-kernel,
	syzbot+f50072212ab792c86925

Hello:

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

On Mon,  9 Mar 2026 21:29:08 +0530 you wrote:
> A race condition exists between lec_atm_close() setting priv->lecd
> to NULL and concurrent access to priv->lecd in send_to_lecd(),
> lec_handle_bridge(), and lec_atm_send(). When the socket is freed
> via RCU while another thread is still using it, a use-after-free
> occurs in sock_def_readable() when accessing the socket's wait queue.
> 
> The root cause is that lec_atm_close() clears priv->lecd without
> any synchronization, while callers dereference priv->lecd without
> any protection against concurrent teardown.
> 
> [...]

Here is the summary with links:
  - [v2] atm: lec: fix use-after-free in sock_def_readable()
    https://git.kernel.org/netdev/net/c/922814879542

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2026-03-14 15:40 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-03-09 15:59 [PATCH v2] atm: lec: fix use-after-free in sock_def_readable() Deepanshu Kartikey
2026-03-10  3:18 ` Hillf Danton
2026-03-12  1:03   ` Deepanshu Kartikey
2026-03-14  2:53     ` Hillf Danton
2026-03-14 12:00 ` Deepanshu Kartikey
2026-03-14 12:10   ` Eric Dumazet
2026-03-14 15:40 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox