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

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

Hi Pei,

I tried this patch on my side and no bug is triggered.

Thanks for your fix!

Best,
Shuangpeng


> On Jun 30, 2026, at 03:40, Pei Xiao <xiaopei01@kylinos.cn> wrote:
> 
> 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@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 | 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)
>