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

[Eric Dumazet]

On Mon, Mar 9, 2026 at 11:34 AM Deepanshu Kartikey
<kartikey406@xxxxxxxxx> wrote:
>
> On Mon, Mar 9, 2026 at 3:29 PM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> >
> > On Mon, Mar 9, 2026 at 10:55 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> > >
> > > On Mon, Mar 9, 2026 at 10:36 AM Deepanshu Kartikey
> > > <kartikey406@xxxxxxxxx> wrote:
> > > >
> > > > A race condition exists between lec_atm_close() setting priv->lecd = 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 holding
> > > > lec_arp_lock, while callers dereference priv->lecd without any protection
> > > > against concurrent teardown.
> > > >
> > > > Fix this by:
> > > > - Protecting priv->lecd = NULL in lec_atm_close() with lec_arp_lock to
> > > >   synchronize with callers that already hold the lock (e.g. lec_arp_resolve)
> > > > - Using sock_hold/sock_put in send_to_lecd() to pin the socket while in
> > > >   use. This is safe because send_to_lecd() is called under lec_arp_lock
> > > >   by lec_arp_resolve(), preventing concurrent NULL assignment of lecd.
> > > > - Using lec_arp_lock + sock_hold/sock_put in lec_handle_bridge() and
> > > >   lec_atm_send() where the lock is not held by the caller, with proper
> > > >   skb cleanup on early exit to avoid memory leaks.
> > > >
> > > > Note: Patch testing via syzbot was attempted but the test VM crashed
> > > > due to a QEMU AHCI emulation assertion failure (hw/ide/core.c:934)
> > > > unrelated to this fix. The QEMU crash is caused by syzbot's disk I/O
> > > > fuzzing triggering a known QEMU NCQ emulation bug, not by this patch.
> > > > Compile testing with "make W=1 net/atm/lec.o" passes cleanly.
> > > >
> > > > Reported-by: syzbot+f50072212ab792c86925@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > > Closes: https://syzkaller.appspot.com/bug?extid=f50072212ab792c86925
> > > > Signed-off-by: Deepanshu Kartikey <kartikey406@xxxxxxxxx>
> > > > ---
> > > >  net/atm/lec.c | 31 +++++++++++++++++++++++++++----
> > > >  1 file changed, 27 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/net/atm/lec.c b/net/atm/lec.c
> > > > index fb93c6e1c329..7e051174a92b 100644
> > > > --- a/net/atm/lec.c
> > > > +++ b/net/atm/lec.c
> > > > @@ -131,6 +131,7 @@ static void lec_handle_bridge(struct sk_buff *skb, struct net_device *dev)
> > > >  {
> > > >         char *buff;
> > > >         struct lec_priv *priv;
> > > > +       unsigned long flags;
> > > >
> > > >         /*
> > > >          * Check if this is a BPDU. If so, ask zeppelin to send
> > > > @@ -154,10 +155,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);
> > > > +               spin_lock_irqsave(&priv->lec_arp_lock, flags);
> > > > +               if (!priv->lecd) {
> > > > +                       spin_unlock_irqrestore(&priv->lec_arp_lock, flags);
> > > > +                       kfree_skb(skb2);
> > > > +                       return;
> > > > +               }
> > > >                 sk = sk_atm(priv->lecd);
> > > > +               sock_hold(sk);
> > > > +               spin_unlock_irqrestore(&priv->lec_arp_lock, flags);
> > >
> > > What prevents priv->lecd to be NULL after you released priv->lec_arp_lock ?
> >
> > More generally, lec_atm_close() clears the sk_receive_queue.
> >
> > So allowing providers to queue more packets would be wrong.
> >
> > So really a better fix is needed.
>
> Thank you for the review Eric.
>
> You are correct on both points.
>
> 1. After releasing lec_arp_lock, priv->lecd can be set to NULL
>    by lec_atm_close() concurrently. I should have saved a local
>    copy of priv->lecd before releasing the lock.
>
> 2. The deeper issue is that lec_atm_close() drains sk_receive_queue
>    but my spinlock approach does not prevent new packets from being
>    queued after the drain, since timer and workqueue paths like
>    lec_arp_work bypass netif_stop_queue().
>
> I think the correct fix would be to use RCU:
> - Mark priv->lecd as __rcu in lec.h
> - Use rcu_assign_pointer() in lec_atm_close() and lecd_attach()
> - Use rcu_read_lock/rcu_dereference in all send paths
> - Use synchronize_rcu() in lec_atm_close() before draining
>   the queue, ensuring all senders have completed before drain
>
> This way synchronize_rcu() guarantees no new packets can be
> queued after the drain begins.
>
> Does this approach look correct to you?

This would be good, make sure lec_atm_close() is allowed to sleep.

If this is vcc_destroy_socket(), maybe sk_receive_queue purging
already happens there.