[v3] char: pcmcia: synclink_cs: Fix use-after-free in mgslpc_ops

[PATCH v3] char: pcmcia: synclink_cs: Fix use-after-free in mgslpc_ops

Posted by Hyunwoo Kim 3 years, 6 months ago

A race condition may occur if the user physically removes
the pcmcia device while calling ioctl() for this tty device node.

This is a race condition between the mgslpc_ioctl() function and
the mgslpc_detach() function, which may eventually result in UAF.

So, add a refcount check to mgslpc_detach() to free the structure
after the tty device node is close()d.

Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
---
 drivers/char/pcmcia/synclink_cs.c | 44 ++++++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/char/pcmcia/synclink_cs.c b/drivers/char/pcmcia/synclink_cs.c
index 8fc49b038372..c991acfbc34d 100644
--- a/drivers/char/pcmcia/synclink_cs.c
+++ b/drivers/char/pcmcia/synclink_cs.c
@@ -216,7 +216,9 @@ typedef struct _mgslpc_info {
 
 	/* PCMCIA support */
 	struct pcmcia_device	*p_dev;
-	int		      stop;
+	int			stop;
+	int			removed;
+	struct kref		refcnt;
 
 	/* SPPP/Cisco HDLC device parts */
 	int netcount;
@@ -228,6 +230,8 @@ typedef struct _mgslpc_info {
 
 } MGSLPC_INFO;
 
+static DEFINE_MUTEX(remove_mutex);
+
 #define MGSLPC_MAGIC 0x5402
 
 /*
@@ -468,10 +472,21 @@ static void mgslpc_wait_until_sent(struct tty_struct *tty, int timeout);
 
 /* PCMCIA prototypes */
 
+static void mgslpc_delete(struct kref *kref);
 static int mgslpc_config(struct pcmcia_device *link);
 static void mgslpc_release(u_long arg);
 static void mgslpc_detach(struct pcmcia_device *p_dev);
 
+static void mgslpc_delete(struct kref *kref)
+{
+	MGSLPC_INFO *info = container_of(kref, MGSLPC_INFO, refcnt);
+	struct pcmcia_device *link = info->p_dev;
+
+	mgslpc_release((u_long)link);
+
+	mgslpc_remove_device(info);
+}
+
 /*
  * 1st function defined in .text section. Calling this function in
  * init_module() followed by a breakpoint allows a remote debugger
@@ -534,6 +549,7 @@ static int mgslpc_probe(struct pcmcia_device *link)
 	init_waitqueue_head(&info->event_wait_q);
 	spin_lock_init(&info->lock);
 	spin_lock_init(&info->netlock);
+	kref_init(&info->refcnt);
 	memcpy(&info->params,&default_params,sizeof(MGSL_PARAMS));
 	info->idle_mode = HDLC_TXIDLE_FLAGS;
 	info->imra_value = 0xffff;
@@ -620,13 +636,17 @@ static void mgslpc_release(u_long arg)
 
 static void mgslpc_detach(struct pcmcia_device *link)
 {
+	MGSLPC_INFO *info = link->priv;
+
+	mutex_lock(&remove_mutex);
 	if (debug_level >= DEBUG_LEVEL_INFO)
 		printk("mgslpc_detach(0x%p)\n", link);
 
-	((MGSLPC_INFO *)link->priv)->stop = 1;
-	mgslpc_release((u_long)link);
+	info->stop = 1;
 
-	mgslpc_remove_device((MGSLPC_INFO *)link->priv);
+	kref_put(&info->refcnt, mgslpc_delete);
+	info->removed = 1;
+	mutex_unlock(&remove_mutex);
 }
 
 static int mgslpc_suspend(struct pcmcia_device *link)
@@ -2341,10 +2361,13 @@ static void mgslpc_close(struct tty_struct *tty, struct file * filp)
 	
 	tty_port_close_end(port, tty);
 	tty_port_tty_set(port, NULL);
+
 cleanup:
 	if (debug_level >= DEBUG_LEVEL_INFO)
 		printk("%s(%d):mgslpc_close(%s) exit, count=%d\n", __FILE__, __LINE__,
 			tty->driver->name, port->count);
+
+	kref_put(&info->refcnt, mgslpc_delete);
 }
 
 /* Wait until the transmitter is empty.
@@ -2465,6 +2488,7 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
 	int		retval, line;
 	unsigned long	flags;
 
+
 	/* verify range of specified line number */
 	line = tty->index;
 	if (line >= mgslpc_device_count) {
@@ -2480,6 +2504,13 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
 	if (mgslpc_paranoia_check(info, tty->name, "mgslpc_open"))
 		return -ENODEV;
 
+	mutex_lock(&remove_mutex);
+
+	if (info->removed == 1) {
+		retval = -ENODEV;
+		goto cleanup;
+	}
+
 	port = &info->port;
 	tty->driver_data = info;
 	tty_port_tty_set(port, tty);
@@ -2517,9 +2548,14 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
 	if (debug_level >= DEBUG_LEVEL_INFO)
 		printk("%s(%d):mgslpc_open(%s) success\n",
 			 __FILE__, __LINE__, info->device_name);
+
+	kref_get(&info->refcnt);
 	retval = 0;
+	mutex_unlock(&remove_mutex);
 
+	return retval;
 cleanup:
+	mutex_unlock(&remove_mutex);
 	return retval;
 }
 
-- 
2.25.1

Re: [PATCH v3] char: pcmcia: synclink_cs: Fix use-after-free in mgslpc_ops

Posted by Ilpo Järvinen 3 years, 6 months ago

On Fri, 16 Sep 2022, Hyunwoo Kim wrote:

> A race condition may occur if the user physically removes
> the pcmcia device while calling ioctl() for this tty device node.
> 
> This is a race condition between the mgslpc_ioctl() function and
> the mgslpc_detach() function, which may eventually result in UAF.
> 
> So, add a refcount check to mgslpc_detach() to free the structure
> after the tty device node is close()d.
> 
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>

> @@ -2517,9 +2548,14 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
>  	if (debug_level >= DEBUG_LEVEL_INFO)
>  		printk("%s(%d):mgslpc_open(%s) success\n",
>  			 __FILE__, __LINE__, info->device_name);
> +
> +	kref_get(&info->refcnt);
>  	retval = 0;
> +	mutex_unlock(&remove_mutex);
>  
> +	return retval;
>  cleanup:
> +	mutex_unlock(&remove_mutex);
>  	return retval;

Just move the cleanup label instead.


-- 
 i.

Re: [PATCH v3] char: pcmcia: synclink_cs: Fix use-after-free in mgslpc_ops

Posted by Hyunwoo Kim 3 years, 6 months ago

On Fri, Sep 16, 2022 at 04:54:11PM +0300, Ilpo Järvinen wrote:
> On Fri, 16 Sep 2022, Hyunwoo Kim wrote:
> 
> > A race condition may occur if the user physically removes
> > the pcmcia device while calling ioctl() for this tty device node.
> > 
> > This is a race condition between the mgslpc_ioctl() function and
> > the mgslpc_detach() function, which may eventually result in UAF.
> > 
> > So, add a refcount check to mgslpc_detach() to free the structure
> > after the tty device node is close()d.
> > 
> > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> 
> > @@ -2517,9 +2548,14 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
> >  	if (debug_level >= DEBUG_LEVEL_INFO)
> >  		printk("%s(%d):mgslpc_open(%s) success\n",
> >  			 __FILE__, __LINE__, info->device_name);
> > +
> > +	kref_get(&info->refcnt);
> >  	retval = 0;
> > +	mutex_unlock(&remove_mutex);
> >  
> > +	return retval;
> >  cleanup:
> > +	mutex_unlock(&remove_mutex);
> >  	return retval;
> 
> Just move the cleanup label instead.

I'm sorry, but could you please explain a bit more?


Best Regards,
Hyunwoo Kim.

Re: [PATCH v3] char: pcmcia: synclink_cs: Fix use-after-free in mgslpc_ops

Posted by Ilpo Järvinen 3 years, 6 months ago

On Fri, 16 Sep 2022, Hyunwoo Kim wrote:

> On Fri, Sep 16, 2022 at 04:54:11PM +0300, Ilpo Järvinen wrote:
> > On Fri, 16 Sep 2022, Hyunwoo Kim wrote:
> > 
> > > A race condition may occur if the user physically removes
> > > the pcmcia device while calling ioctl() for this tty device node.
> > > 
> > > This is a race condition between the mgslpc_ioctl() function and
> > > the mgslpc_detach() function, which may eventually result in UAF.
> > > 
> > > So, add a refcount check to mgslpc_detach() to free the structure
> > > after the tty device node is close()d.
> > > 
> > > Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> > 
> > > @@ -2517,9 +2548,14 @@ static int mgslpc_open(struct tty_struct *tty, struct file * filp)
> > >  	if (debug_level >= DEBUG_LEVEL_INFO)
> > >  		printk("%s(%d):mgslpc_open(%s) success\n",
> > >  			 __FILE__, __LINE__, info->device_name);
> > > +
> > > +	kref_get(&info->refcnt);
> > >  	retval = 0;
> > > +	mutex_unlock(&remove_mutex);
> > >  
> > > +	return retval;
> > >  cleanup:
> > > +	mutex_unlock(&remove_mutex);
> > >  	return retval;
> > 
> > Just move the cleanup label instead.
> 
> I'm sorry, but could you please explain a bit more?

The last two statements of the cleanup path and the path above it are
the same.

-- 
 i.