Re: [PATCH 1/2] ipack: ipoctal: fix UAF, null-ptr-deref, and use-after-free in cleanup on remove

[Pei Xiao]

hi shuangpeng,

 If this version have no bugs. I will send it to Greg for review.
 Thanks!

在 2026/6/26 10:31, Pei Xiao 写道:
> Three issues arise when the device is removed while a tty session is
> still active:
>
> 1. UAF of struct ipoctal: the remove callback frees ipoctal via
>    kfree() while tty ops may still access it.  Fix by introducing
>    kref-based lifetime management — kref is taken in install() when
>    a tty is opened and released in cleanup() when the tty is finally
>    destroyed; remove() uses kref_put() instead of kfree().
>
> 2. NULL dereference in ipoctal_write_tty(): __ipoctal_remove()
>    frees xmit_buf via tty_port_free_xmit_buf() while a userspace
>    process may still hold the tty fd and call write().  Fix by
>    checking for NULL xmit_buf in ipoctal_write_tty().
>
> 3. UAF in ipoctal_cleanup(): ipack_put_carrier(ipoctal->dev)
>    dereferences ipoctal->dev after the ipack_device has been freed
>    by ipack_device_del().  Fix by caching ipoctal->carrier_owner
>    during probe() and calling module_put() on the cached pointer
>    directly in cleanup(), avoiding any access to ipoctal->dev.
>
> Also introduce a "removed" flag in struct ipoctal, set at the start
> of __ipoctal_remove(), and checked in every tty op that accesses
> hardware resources (port_activate, write_tty, set_termios, hangup,
> shutdown).  This prevents page faults when devm_ioremap() regions
> are unmapped after remove() returns.
>
> Reported-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
> Closes: https://lore.kernel.org/lkml/178144969601.60470.1257088106279546587@xxxxxxxxx/
> Fixes: 05e5027efc9c ("Staging: ipack: move out of staging")
> Signed-off-by: Pei Xiao <xiaopei01@xxxxxxxxxx>
> ---
>  drivers/ipack/devices/ipoctal.c | 56 ++++++++++++++++++++++++++++++---
>  1 file changed, 52 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/ipack/devices/ipoctal.c b/drivers/ipack/devices/ipoctal.c
> index 1bbefc6de708..bf71b8952a7c 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,9 @@ struct ipoctal {
>  	struct tty_driver		*tty_drv;
>  	u8 __iomem			*mem8_space;
>  	u8 __iomem			*int_space;
> +	struct kref			kref;
> +	struct module			*carrier_owner;
> +	bool				removed;
>  };
>  
>  static inline struct ipoctal *chan_to_ipoctal(struct ipoctal_channel *chan,
> @@ -70,8 +76,14 @@ 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 +107,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 +473,13 @@ 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 +519,13 @@ 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 +654,16 @@ 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 +680,16 @@ 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);
>  }
> @@ -664,8 +699,9 @@ static void ipoctal_cleanup(struct tty_struct *tty)
>  	struct ipoctal_channel *channel = tty->driver_data;
>  	struct ipoctal *ipoctal = chan_to_ipoctal(channel, tty->index);
>  
> -	/* release the carrier driver */
> -	ipack_put_carrier(ipoctal->dev);
> +	/* release the carrier driver via cached owner */
> +	module_put(ipoctal->carrier_owner);
> +	kref_put(&ipoctal->kref, ipoctal_release);
>  }
>  
>  static const struct tty_operations ipoctal_fops = {
> @@ -683,6 +719,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,7 +735,10 @@ static int ipoctal_probe(struct ipack_device *dev)
>  	if (ipoctal == NULL)
>  		return -ENOMEM;
>  
> +	kref_init(&ipoctal->kref);
> +
>  	ipoctal->dev = dev;
> +	ipoctal->carrier_owner = dev->bus->owner;
>  	res = ipoctal_inst_slot(ipoctal, dev->bus->bus_nr, dev->slot);
>  	if (res)
>  		goto out_uninst;
> @@ -701,7 +747,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 +755,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 +773,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)