| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aXUZJ_xO2hGAvV6-@osama>
Date: Sat, 24 Jan 2026 20:10:31 +0100
From: Osama Abdelkader <osama.abdelkader@...il.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Simon Horman <horms@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sjur Braendeland <sjur.brandeland@...ricsson.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
syzbot+f9d847b2b84164fa69f3@...kaller.appspotmail.com,
stable@...r.kernel.org
Subject: Re: [PATCH v2] net: caif: fix memory leak in ldisc_receive
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@...kaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=f9d847b2b84164fa69f3
> > > Fixes: 9b27105b4a44 ("net-caif-driver: add CAIF serial driver (ldisc)")
> > > CC: stable@...r.kernel.org
> > > Signed-off-by: Osama Abdelkader <osama.abdelkader@...il.com>
> > > ---
> > > 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);
Powered by blists - more mailing lists