Re: [PATCH] ipack: ipoctal: fix use-after-free and null-ptr-deref on remove

[Shuangpeng <shuangpeng.kernel@gmail.com>]

Hi Pei,

I applied the new patch and reran the same reproducer [1]. It triggers
another KASAN report:

[ 63.599954][ T1151] BUG: KASAN: slab-use-after-free in ipoctal_cleanup (include/linux/ipack.h:292 drivers/ipack/devices/ipoctal.c:697)
[ 63.600513][ T1151] Read of size 8 at addr ffff8881136e5008 by task kworker/1:2/1151
[ 63.601248][ T1151] Hardware name: QEMU Ubuntu 24.04 PC v2 (i440FX + PIIX, arch_caps fix, 1996), BIOS 1.16.3-debian-1.16.3-2 04/01/2014
[ 63.601249][ T1151] Workqueue: events release_one_tty
[ 63.601254][ T1151] Call Trace:
[ 63.601257][ T1151]  <TASK>
[ 63.601258][ T1151]  dump_stack_lvl (lib/dump_stack.c:94 lib/dump_stack.c:120)
[ 63.601261][ T1151]  print_report (mm/kasan/report.c:378 mm/kasan/report.c:482)
[ 63.601272][ T1151]  kasan_report (mm/kasan/report.c:595)
[ 63.601286][ T1151]  ipoctal_cleanup (include/linux/ipack.h:292 drivers/ipack/devices/ipoctal.c:697)
[ 63.601289][ T1151]  release_one_tty (drivers/tty/tty_io.c:1522)
[ 63.601292][ T1151]  process_scheduled_works (kernel/workqueue.c:3322 kernel/workqueue.c:3405)
[ 63.601297][ T1151]  worker_thread (kernel/workqueue.c:3486)
[ 63.601303][ T1151]  kthread (kernel/kthread.c:436)
[ 63.601310][ T1151]  ret_from_fork (kernel/process.c:158)
[ 63.601320][ T1151]  ret_from_fork_asm (arch/x86/entry/entry_64.S:245)
[ 63.601324][ T1151]  </TASK>
[ 63.610982][ T1151] The buggy address belongs to the object at ffff8881136e5000
[ 63.610982][ T1151]  which belongs to the cache kmalloc-1k of size 1024
[ 63.611938][ T1151] The buggy address is located 8 bytes inside of
[ 63.611938][ T1151]  freed 1024-byte region [ffff8881136e5000, ffff8881136e5400)

I am also happy to test the next version if needed.

Best,
Shuangpeng

[1] https://gist.github.com/shuangpengbai/bc71ccd7ce1454abae45897fdde6813b


> On Jun 24, 2026, at 23:24, Pei Xiao <xiaopei01@kylinos.cn> wrote:
> 
> A use-after-free occurs when the device is removed while a tty
> session is still active.  The remove callback frees the ipoctal
> structure via kfree() while tty ops may still access it.
> 
> Fix this by introducing kref-based lifetime management. The kref is
> initialized in probe(), taken in ipoctal_install() when a tty is
> opened, and released in ipoctal_cleanup() when the tty is finally
> destroyed.  The remove callback replaces direct kfree() with
> kref_put(), ensuring the memory is freed only after all references
> are released.
> 
> However, the kref keeps only the ipoctal struct alive — the MMIO
> mappings (devm_ioremap) and xmit_buf are released during remove()
> regardless.  If a tty fd is still open:
> 
>  - tty_port_free_xmit_buf() NULLs xmit_buf, leading to a NULL
>    dereference in ipoctal_write_tty().
>  - devm iounmap() removes page table mappings, leading to a page
>    fault in ipoctal_set_termios() and other tty ops that access
>    channel->regs via iowrite8().
> 
> Fix both by:
> 
>  (1) checking for NULL xmit_buf in ipoctal_write_tty(), and
>  (2) introducing a "removed" flag in struct ipoctal, set at the
>      start of __ipoctal_remove(), and checked in every tty op that
>      touches hardware registers (write_tty, set_termios,
>      port_activate, hangup, shutdown).
> 
> Reported-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
> Closes: https://lore.kernel.org/lkml/178144969601.60470.1257088106279546587@gmail.com/
> Fixes: 05e5027efc9c ("Staging: ipack: move out of staging")
> Signed-off-by: Pei Xiao <xiaopei01@kylinos.cn>
> ---
> drivers/ipack/devices/ipoctal.c | 45 +++++++++++++++++++++++++++++++--
> 1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
> index 1bbefc6de708..2b9d381cf56d 100644
> --- a/drivers/ipack/devices/ipoctal.c
> +++ b/drivers/ipack/devices/ipoctal.c
> @@ -10,6 +10,7 @@
> #include <linux/device.h>
> #include <linux/module.h>
> #include <linux/interrupt.h>
> +#include <linux/kref.h>
> #include <linux/sched.h>
> #include <linux/tty.h>
> #include <linux/serial.h>
> @@ -25,6 +26,8 @@
> 
> static const struct tty_operations ipoctal_fops;
> 
> +static void ipoctal_release(struct kref *kref);
> +
> struct ipoctal_channel {
> struct ipoctal_stats stats;
> unsigned int nb_bytes;
> @@ -49,6 +52,8 @@ struct ipoctal {
> struct tty_driver *tty_drv;
> u8 __iomem *mem8_space;
> u8 __iomem *int_space;
> + struct kref kref;
> + bool removed;
> };
> 
> static inline struct ipoctal *chan_to_ipoctal(struct ipoctal_channel *chan,
> @@ -70,8 +75,13 @@ static void ipoctal_reset_channel(struct ipoctal_channel *channel)
> static int ipoctal_port_activate(struct tty_port *port, struct tty_struct *tty)
> {
> struct ipoctal_channel *channel;
> + struct ipoctal *ipoctal;
> 
> channel = dev_get_drvdata(tty->dev);
> + ipoctal = chan_to_ipoctal(channel, tty->index);
> +
> + if (ipoctal->removed)
> + return -ENODEV;
> 
> /*
> * Enable RX. TX will be enabled when
> @@ -95,6 +105,7 @@ static int ipoctal_install(struct tty_driver *driver, struct tty_struct *tty)
> if (res)
> goto err_put_carrier;
> 
> + kref_get(&ipoctal->kref);
> tty->driver_data = channel;
> 
> return 0;
> @@ -460,8 +471,12 @@ static ssize_t ipoctal_write_tty(struct tty_struct *tty, const u8 *buf,
> size_t count)
> {
> struct ipoctal_channel *channel = tty->driver_data;
> + struct ipoctal *ipoctal = chan_to_ipoctal(channel, tty->index);
> size_t char_copied;
> 
> + if (ipoctal->removed || !channel->tty_port.xmit_buf)
> + return 0;
> +
> char_copied = ipoctal_copy_write_buffer(channel, buf, count);
> 
> /* As the IP-OCTAL 485 only supports half duplex, do it manually */
> @@ -501,8 +516,12 @@ static void ipoctal_set_termios(struct tty_struct *tty,
> unsigned char mr2 = 0;
> unsigned char csr = 0;
> struct ipoctal_channel *channel = tty->driver_data;
> + struct ipoctal *ipoctal = chan_to_ipoctal(channel, tty->index);
> speed_t baud;
> 
> + if (ipoctal->removed)
> + return;
> +
> cflag = tty->termios.c_cflag;
> 
> /* Disable and reset everything before change the setup */
> @@ -631,10 +650,15 @@ static void ipoctal_hangup(struct tty_struct *tty)
> {
> unsigned long flags;
> struct ipoctal_channel *channel = tty->driver_data;
> + struct ipoctal *ipoctal;
> 
> if (channel == NULL)
> return;
> 
> + ipoctal = chan_to_ipoctal(channel, tty->index);
> + if (ipoctal->removed)
> + return;
> +
> spin_lock_irqsave(&channel->lock, flags);
> channel->nb_bytes = 0;
> channel->pointer_read = 0;
> @@ -651,10 +675,15 @@ static void ipoctal_hangup(struct tty_struct *tty)
> static void ipoctal_shutdown(struct tty_struct *tty)
> {
> struct ipoctal_channel *channel = tty->driver_data;
> + struct ipoctal *ipoctal;
> 
> if (channel == NULL)
> return;
> 
> + ipoctal = chan_to_ipoctal(channel, tty->index);
> + if (ipoctal->removed)
> + return;
> +
> ipoctal_reset_channel(channel);
> tty_port_set_initialized(&channel->tty_port, false);
> }
> @@ -666,6 +695,7 @@ static void ipoctal_cleanup(struct tty_struct *tty)
> 
> /* release the carrier driver */
> ipack_put_carrier(ipoctal->dev);
> + kref_put(&ipoctal->kref, ipoctal_release);
> }
> 
> static const struct tty_operations ipoctal_fops = {
> @@ -683,6 +713,13 @@ static const struct tty_operations ipoctal_fops = {
> .cleanup =              ipoctal_cleanup,
> };
> 
> +static void ipoctal_release(struct kref *kref)
> +{
> + struct ipoctal *ipoctal = container_of(kref, struct ipoctal, kref);
> +
> + kfree(ipoctal);
> +}
> +
> static int ipoctal_probe(struct ipack_device *dev)
> {
> int res;
> @@ -692,6 +729,8 @@ static int ipoctal_probe(struct ipack_device *dev)
> if (ipoctal == NULL)
> return -ENOMEM;
> 
> + kref_init(&ipoctal->kref);
> +
> ipoctal->dev = dev;
> res = ipoctal_inst_slot(ipoctal, dev->bus->bus_nr, dev->slot);
> if (res)
> @@ -701,7 +740,7 @@ static int ipoctal_probe(struct ipack_device *dev)
> return 0;
> 
> out_uninst:
> - kfree(ipoctal);
> + kref_put(&ipoctal->kref, ipoctal_release);
> return res;
> }
> 
> @@ -709,6 +748,8 @@ static void __ipoctal_remove(struct ipoctal *ipoctal)
> {
> int i;
> 
> + ipoctal->removed = true;
> +
> ipoctal->dev->bus->ops->free_irq(ipoctal->dev);
> 
> for (i = 0; i < NR_CHANNELS; i++) {
> @@ -725,7 +766,7 @@ static void __ipoctal_remove(struct ipoctal *ipoctal)
> tty_unregister_driver(ipoctal->tty_drv);
> kfree(ipoctal->tty_drv->name);
> tty_driver_kref_put(ipoctal->tty_drv);
> - kfree(ipoctal);
> + kref_put(&ipoctal->kref, ipoctal_release);
> }
> 
> static void ipoctal_remove(struct ipack_device *idev)
> -- 
> 2.25.1
>