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

[Jiayuan Chen]

February 25, 2026 at 18:58, "Dan Carpenter" <dan.carpenter@xxxxxxxxxx mailto:dan.carpenter@xxxxxxxxxx?to=%22Dan%20Carpenter%22%20%3Cdan.carpenter%40linaro.org%3E > wrote:


> 
> 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.



Hi Dan,

Thanks for the review. You're right, the entry->recv_vcc->push to vcc->push rename should not be mixed into the bug fix patch.

I think I should only adds the if (vpriv) guards without any other changes.

I'll drop the cleanup rename entirely since this is legacy code and not worth a separate patch.

Best,
Jiayuan

> regards,
> dan carpenter
>