Re: [PATCH net v1] atm: lec: fix null-ptr-deref in lec_arp_clear_vccs

[Dan Carpenter]

On Tue, Feb 24, 2026 at 12:46:38PM +0800, Jiayuan Chen wrote:
> From: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> 
> syzkaller reported a null-ptr-deref in lec_arp_clear_vccs().
> This issue can be easily reproduced using the syzkaller reproducer.
> 
> In the ATM LANE (LAN Emulation) module, the same atm_vcc can be shared by
> multiple lec_arp_table entries (e.g., via entry->vcc or entry->recv_vcc).
> When the underlying VCC is closed, lec_vcc_close() iterates over all
> ARP entries and calls lec_arp_clear_vccs() for each matched entry.
> 
> For example, when lec_vcc_close() iterates through the hlists in
> priv->lec_arp_empty_ones or other ARP tables:
> 
> 1. In the first iteration, for the first matched ARP entry sharing the VCC,
> lec_arp_clear_vccs() frees the associated vpriv (which is vcc->user_back)
> and sets vcc->user_back to NULL.
> 2. In the second iteration, for the next matched ARP entry sharing the same
> VCC, lec_arp_clear_vccs() is called again. It obtains a NULL vpriv from
> vcc->user_back (via LEC_VCC_PRIV(vcc)) and then attempts to dereference it
> via `vcc->pop = vpriv->old_pop`, leading to a null-ptr-deref crash.
> 
> Fix this by adding a null check for vpriv before dereferencing it. If
> vpriv is already NULL, it means the VCC has been cleared by a previous
> call, so we can safely skip the cleanup and just clear the entry's
> vcc/recv_vcc pointers. Note that the added check is intentional and
> necessary to avoid calling vcc_release_async() multiple times on the
> same vcc/recv_vcc, not just protecting the kfree().
> 
> Reported-by: syzbot+72e3ea390c305de0e259@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://lore.kernel.org/all/68c95a83.050a0220.3c6139.0e5c.GAE@xxxxxxxxxx/T/
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jiayuan Chen <jiayuan.chen@xxxxxxxxxx>
> ---
>  net/atm/lec.c | 27 +++++++++++++++------------
>  1 file changed, 15 insertions(+), 12 deletions(-)
> 
> diff --git a/net/atm/lec.c b/net/atm/lec.c
> index afb8d3eb2185..a5b80d6df603 100644
> --- a/net/atm/lec.c
> +++ b/net/atm/lec.c
> @@ -1260,24 +1260,27 @@ static void lec_arp_clear_vccs(struct lec_arp_table *entry)
>  		struct lec_vcc_priv *vpriv = LEC_VCC_PRIV(vcc);
>  		struct net_device *dev = (struct net_device *)vcc->proto_data;
>  
> -		vcc->pop = vpriv->old_pop;
> -		if (vpriv->xoff)
> -			netif_wake_queue(dev);
> -		kfree(vpriv);
> -		vcc->user_back = NULL;
> -		vcc->push = entry->old_push;
> -		vcc_release_async(vcc, -EPIPE);
> +		if (vpriv) {
> +			vcc->pop = vpriv->old_pop;
> +			if (vpriv->xoff)
> +				netif_wake_queue(dev);
> +			kfree(vpriv);
> +			vcc->user_back = NULL;
> +			vcc->push = entry->old_push;
> +			vcc_release_async(vcc, -EPIPE);
> +		}
>  		entry->vcc = NULL;
>  	}
>  	if (entry->recv_vcc) {
>  		struct atm_vcc *vcc = entry->recv_vcc;
>  		struct lec_vcc_priv *vpriv = LEC_VCC_PRIV(vcc);
>  
> -		kfree(vpriv);
> -		vcc->user_back = NULL;
> -
> -		entry->recv_vcc->push = entry->old_recv_push;
> -		vcc_release_async(entry->recv_vcc, -EPIPE);
> +		if (vpriv) {
> +			kfree(vpriv);
> +			vcc->user_back = NULL;
> +			vcc->push = entry->old_recv_push;
> +			vcc_release_async(vcc, -EPIPE);

I wasn't going say anything, but since it seems like maybe you're going
to redo this patch anyway.  Changing "entry->recv_vcc->push" to
"vcc->push" is obviously nice but could you do that in a separate
patch?

I use a tool to strip out the indenting changes:
https://github.com/error27/rename_rev

So when I review a patch like this, I want to pipe it to my script and
just see:

+		if (vpriv) {
			vcc->pop = vpriv->old_pop;
			if (vpriv->xoff)
				netif_wake_queue(dev);
			kfree(vpriv);
			vcc->user_back = NULL;
			vcc->push = entry->old_push;
			vcc_release_async(vcc, -EPIPE);
+		}
  		entry->vcc = NULL;
  	}
  	if (entry->recv_vcc) {
  		struct atm_vcc *vcc = entry->recv_vcc;
  		struct lec_vcc_priv *vpriv = LEC_VCC_PRIV(vcc);

+		if (vpriv) {
			kfree(vpriv);
			vcc->user_back = NULL;

			entry->recv_vcc->push = entry->old_recv_push;
			vcc_release_async(entry->recv_vcc, -EPIPE);
+		}

The renames are nice but now I have to check things by hand.

regards,
dan carpenter