Re: [PATCH v2] net: caif: fix memory leak in ldisc_receive

[Osama Abdelkader]

On Mon, Jan 19, 2026 at 08:00:37AM +0100, Jiri Slaby wrote:
> On 19. 01. 26, 7:58, Jiri Slaby wrote:
> > On 18. 01. 26, 18:44, Osama Abdelkader wrote:
> > > Add NULL pointer checks for ser and ser->dev in ldisc_receive() to
> > > prevent memory leaks when the function is called during device close
> > > or in race conditions where tty->disc_data or ser->dev may be NULL.
> > > 
> > > The memory leak occurred because ser->dev was accessed before checking
> > > if ser or ser->dev was NULL, which could cause a NULL pointer
> > > dereference or use of freed memory. Additionally, set tty->disc_data
> > > to NULL in ldisc_close() to prevent receive_buf() from using a freed
> > > ser pointer after the line discipline is closed.
> > > 
> > > Reported-by: syzbot+f9d847b2b84164fa69f3@xxxxxxxxxxxxxxxxxxxxxxxxx
> > > Closes: https://syzkaller.appspot.com/bug?extid=f9d847b2b84164fa69f3
> > > Fixes: 9b27105b4a44 ("net-caif-driver: add CAIF serial driver (ldisc)")
> > > CC: stable@xxxxxxxxxxxxxxx
> > > Signed-off-by: Osama Abdelkader <osama.abdelkader@xxxxxxxxx>
> > > ---
> > > v2:
> > > 1.Combine NULL pointer checks for ser and ser->dev in ldisc_receive()
> > > 2.Set tty->disc_data = NULL in ldisc_close() to prevent receive_buf()
> > > from using a freed ser pointer after close.
> > > 3.Add NULL pointer check for ser in ldisc_close()
> > > ---
> > >   drivers/net/caif/caif_serial.c | 8 ++++++--
> > >   1 file changed, 6 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/caif/caif_serial.c b/drivers/net/caif/
> > > caif_serial.c
> > > index c398ac42eae9..970237a3ccca 100644
> > > --- a/drivers/net/caif/caif_serial.c
> > > +++ b/drivers/net/caif/caif_serial.c
> > > @@ -152,6 +152,8 @@ static void ldisc_receive(struct tty_struct
> > > *tty, const u8 *data,
> > >       int ret;
> > >       ser = tty->disc_data;
> > > +    if (!ser || !ser->dev)
> > > +        return;
> > >       /*
> > >        * NOTE: flags may contain information about break or overrun.
> > > @@ -170,8 +172,6 @@ static void ldisc_receive(struct tty_struct
> > > *tty, const u8 *data,
> > >           return;
> > >       }
> > > -    BUG_ON(ser->dev == NULL);
> > > -
> > >       /* Get a suitable caif packet and copy in data. */
> > >       skb = netdev_alloc_skb(ser->dev, count+1);
> > 
> > Wait, the reported error is memory leak of mem allocated here. So both
> > ser and ser->dev appear to be valid?
> > 
> > So instead, does netif_rx() return an error few lines below for some
> > reason? So should skb just be freed in that path?
> > 
> >          if (skb == NULL)
> >                  return;
> >          skb_put_data(skb, data, count);
> > 
> >          skb->protocol = htons(ETH_P_CAIF);
> >          skb_reset_mac_header(skb);
> >          debugfs_rx(ser, data, count);
> >          /* Push received packet up the stack. */
> >          ret = netif_rx(skb);
> >          if (!ret) {
> >                  ser->dev->stats.rx_packets++;
> >                  ser->dev->stats.rx_bytes += count;
> >          } else
> >                  ++ser->dev->stats.rx_dropped;
> > 
> > Not calling skb_free() in this else path _is_ a BUG™ in any case IMO.
> 
> No, it's not: netif_rx() always eats the buffer. So care to elaborate why
> the socket buffer is actually leaked?
> 
> > thanks,
> -- 
> js
> suse labs
> 

Hi Jiri, Greg

Following up on v2 feedback regarding the CAIF close-receive race,
I'm thinking of (similar to PPP driver):

- Add proper lifetime management using a refcount (`rx_ref`) to track
  in-flight receive callbacks.
- Introduce a `closing` flag to prevent new receive callbacks after ldisc_close()
  starts.
- Use a completion (`rx_done`) to wait for in-flight callbacks to finish before 
  freeing resources.

I plan to verify the patch using the ReproC program and kmemleak locally, but I
would appreciate feedback on this approach before testing.

proposal is written below.

Thanks for your review!

Best regards,
Osama 

--- a/drivers/net/caif/caif_serial.c
+++ b/drivers/net/caif/caif_serial.c
@@ struct ser_device {
     /* existing fields */
+    refcount_t rx_ref;             /* track in-flight receive callbacks */
+    bool closing;                  /* indicates ldisc_close in progress */
+    spinlock_t lock;               /* protects closing flag */
+    struct completion rx_done;     /* signals all RX finished */
 };
 
@@ static void ldisc_receive(struct tty_struct *tty,
     struct ser_device *ser = tty->disc_data;
 
-    if (!ser)
-        return;
+    if (!ser)
+        return; /* safety check, usually handled by TTY core */
+
+    /* Acquire lock to check closing */
+    spin_lock(&ser->lock);
+    if (ser->closing) {
+        spin_unlock(&ser->lock);
+        return;
+    }
+    refcount_inc(&ser->rx_ref);
+    spin_unlock(&ser->lock);
+
+    /* existing RX processing code */
+
+    /* RX done */
+    if (refcount_dec_and_test(&ser->rx_ref))
+        complete(&ser->rx_done);
 }
 
@@ static void ldisc_close(struct tty_struct *tty)
     struct ser_device *ser = tty->disc_data;
 
     if (!ser)
         return;
+
+    /* mark closing and prevent new receive callbacks */
+    spin_lock(&ser->lock);
+    ser->closing = true;
+    spin_unlock(&ser->lock);
+
+    /* wait for in-flight RX to finish */
+    wait_for_completion(&ser->rx_done);
+
+    /* safe to free ser and associated resources now */
+
+    /* optional: re-init completion for future opens */
+    init_completion(&ser->rx_done);