Message-ID: <63030af8-5849-34b3-10e6-b6ce32c3a5bf@kernel.org>
Date:   Tue, 21 Feb 2023 07:51:46 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.7.1
Subject: Re: [PATCH v3] char: pcmcia: cm4000_cs: Fix use-after-free in
 cm4000_fops
Content-Language: en-US
To:     Hyunwoo Kim <imv4bel@gmail.com>, laforge@gnumonks.org
Cc:     linux-kernel@vger.kernel.org, arnd@arndb.de,
        gregkh@linuxfoundation.org, linux@dominikbrodowski.net
References: <20220919040701.GA302806@ubuntu>
From:   Jiri Slaby <jirislaby@kernel.org>
In-Reply-To: <20220919040701.GA302806@ubuntu>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

Ping -- what's the status of these?

Should we mark cm4000_cs, cm4040_cs, and scr24x_cs as BROKEN instead?

Thanks.

On 19. 09. 22, 6:07, Hyunwoo Kim wrote:
> A race condition may occur if the user physically removes the pcmcia
> device while calling open() for this char device node.
> 
> This is a race condition between the cmm_open() function and the
> cm4000_detach() function, which may eventually result in UAF.
> 
> So, add a refcount check to cm4000_detach() to free the "dev" structure
> after the char device node is close()d.
> 
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>   drivers/char/pcmcia/cm4000_cs.c | 58 +++++++++++++++++++++++----------
>   1 file changed, 41 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index adaec8fd4b16..7103812b4019 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -55,6 +55,7 @@
>   	} while (0)
>   
>   static DEFINE_MUTEX(cmm_mutex);
> +static DEFINE_MUTEX(remove_mutex);
>   
>   #define	T_1SEC		(HZ)
>   #define	T_10MSEC	msecs_to_jiffies(10)
> @@ -103,7 +104,8 @@ static int major;		/* major number we get from the kernel */
>   #define REG_STOPBITS(x)		(x + 7)
>   
>   struct cm4000_dev {
> -	struct pcmcia_device *p_dev;
> +	struct pcmcia_device	*p_dev;
> +	struct kref		refcnt;
>   
>   	unsigned char atr[MAX_ATR];
>   	unsigned char rbuf[512];
> @@ -146,6 +148,9 @@ struct cm4000_dev {
>   
>   #define	ZERO_DEV(dev)	memset(&((dev)->init), 0, sizeof((dev)->init))
>   
> +static void stop_monitor(struct cm4000_dev *dev);
> +static void cm4000_delete(struct kref *kref);
> +
>   static struct pcmcia_device *dev_table[CM4000_MAX_DEV];
>   static struct class *cmm_class;
>   
> @@ -416,6 +421,30 @@ static struct card_fixup card_fixups[] = {
>   	},
>   };
>   
> +
> +static void cm4000_delete(struct kref *kref)
> +{
> +	struct cm4000_dev *dev = container_of(kref, struct cm4000_dev, refcnt);
> +	struct pcmcia_device *link = dev->p_dev;
> +	int devno;
> +
> +	/* find device */
> +	for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> +		if (dev_table[devno] == link)
> +			break;
> +	if (devno == CM4000_MAX_DEV)
> +		return;
> +
> +	stop_monitor(dev);
> +
> +	cm4000_release(link);
> +
> +	dev_table[devno] = NULL;
> +	kfree(dev);
> +
> +	device_destroy(cmm_class, MKDEV(major, devno));
> +}
> +
>   static void set_cardparameter(struct cm4000_dev *dev)
>   {
>   	int i;
> @@ -1629,6 +1658,7 @@ static int cmm_open(struct inode *inode, struct file *filp)
>   	if (minor >= CM4000_MAX_DEV)
>   		return -ENODEV;
>   
> +	mutex_lock(&remove_mutex);
>   	mutex_lock(&cmm_mutex);
>   	link = dev_table[minor];
>   	if (link == NULL || !pcmcia_dev_present(link)) {
> @@ -1673,8 +1703,12 @@ static int cmm_open(struct inode *inode, struct file *filp)
>   
>   	DEBUGP(2, dev, "<- cmm_open\n");
>   	ret = stream_open(inode, filp);
> +
> +	kref_get(&dev->refcnt);
>   out:
>   	mutex_unlock(&cmm_mutex);
> +	mutex_unlock(&remove_mutex);
> +
>   	return ret;
>   }
>   
> @@ -1703,6 +1737,8 @@ static int cmm_close(struct inode *inode, struct file *filp)
>   	link->open = 0;		/* only one open per device */
>   	wake_up(&dev->devq);	/* socket removed? */
>   
> +	kref_put(&dev->refcnt, cm4000_delete);
> +
>   	DEBUGP(2, dev, "cmm_close\n");
>   	return 0;
>   }
> @@ -1808,6 +1844,7 @@ static int cm4000_probe(struct pcmcia_device *link)
>   	init_waitqueue_head(&dev->ioq);
>   	init_waitqueue_head(&dev->atrq);
>   	init_waitqueue_head(&dev->readq);
> +	kref_init(&dev->refcnt);
>   
>   	ret = cm4000_config(link, i);
>   	if (ret) {
> @@ -1824,23 +1861,10 @@ static int cm4000_probe(struct pcmcia_device *link)
>   static void cm4000_detach(struct pcmcia_device *link)
>   {
>   	struct cm4000_dev *dev = link->priv;
> -	int devno;
> -
> -	/* find device */
> -	for (devno = 0; devno < CM4000_MAX_DEV; devno++)
> -		if (dev_table[devno] == link)
> -			break;
> -	if (devno == CM4000_MAX_DEV)
> -		return;
> -
> -	stop_monitor(dev);
> -
> -	cm4000_release(link);
>   
> -	dev_table[devno] = NULL;
> -	kfree(dev);
> -
> -	device_destroy(cmm_class, MKDEV(major, devno));
> +	mutex_lock(&remove_mutex);
> +	kref_put(&dev->refcnt, cm4000_delete);
> +	mutex_unlock(&remove_mutex);
>   
>   	return;
>   }

-- 
js
suse labs