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

[Simon Horman <horms@kernel.org>]

On Wed, Feb 25, 2026 at 10:16:40AM +0000, Jiayuan Chen wrote:
> February 25, 2026 at 17:45, "Simon Horman" <horms@kernel.org mailto:horms@kernel.org?to=%22Simon%20Horman%22%20%3Chorms%40kernel.org%3E > wrote:
> 
> 
> > 
> > On Wed, Feb 25, 2026 at 08:37:05AM +0000, Simon Horman wrote:
> > 
> > > 
> > > On Tue, Feb 24, 2026 at 12:46:38PM +0800, Jiayuan Chen wrote:
> > >  From: Jiayuan Chen <jiayuan.chen@shopee.com>
> > >  
> > >  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().
> > > 
> > Sorry for coming back to this a 2nd time.
> > After thinking about this some more I'd like to pass on
> > some feedback from the AI powered review.
> > 
> > I'll put the full text below. But in a nutshell: could you clarify
> > why it is necessary to avoid calling vcc_release_async() multiple times.
> 
> 
> Hi Simon,
> 
> Thanks for the feedback.
> 
> 1. Regarding why it is necessary to avoid calling vcc_release_async()
> multiple times: when vpriv is NULL, it means a previous call to
> lec_arp_clear_vccs() has already freed vpriv, set vcc->user_back = NULL,
> restored vcc->push, and called vcc_release_async() for this VCC.  Calling
> vcc_release_async() again on an already-released VCC would redundantly
> set flags, set sk_err, and trigger sk_state_change() on a socket that is
> already shutting down. While this may not cause an immediate crash, it is
> semantically wrong — the VCC has already been properly released, and we
> should not repeat the teardown sequence.
> 
> That is why I intentionally placed the entire cleanup block (including
> vcc_release_async()) inside the if (vpriv) guard, rather than only
> guarding the vpriv dereference. The NULL vpriv serves as a reliable
> indicator that this VCC has already been processed by a prior iteration.

Thanks. I think it would be useful to include an explanation along
those lines in the commit message.

> 2. Regarding the Fixes tag: the AI suggests pointing to 8d9f73c0ad2f
> ("atm: fix a memory leak of vcc->user_back"), but that only introduced
> the entry->recv_vcc cleanup path. The entry->vcc path has had the same
> bug since the beginning — if two ARP entries share the same VCC, the
> second call dereferences NULL vpriv via vcc->pop = vpriv->old_pop.  My
> patch fixes both paths, and the entry->vcc path bug traces back to the
> original code, so Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") is correct.
> I've verified that the entry->vcc path is independently triggerable with
> a seperated reproducer [1] (it's not worth to put it into selftest for
> legacy module)
> 
> I also don't think splitting this into two patches makes sense — both
> paths are in the same function, exhibit the same bug pattern, and the fix
> is identical.

Understood. I agree the code changes make sense to keep together
in a single patch. And retain the Fixes tag as you have it.

...