Re: [PATCH v2] atm: lec: fix use-after-free in sock_def_readable()

[Hillf Danton <hdanton@sina.com>]

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