linux-kernel - [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sun,  2 Jan 2022 21:01:40 -0800
From:   Daniel Xu <dxu@...uu.xyz>
To:     arnd@...db.de, gregkh@...uxfoundation.org, giometti@...eenne.com,
        linux-kernel@...r.kernel.org
Cc:     Daniel Xu <dxu@...uu.xyz>, thesven73@...il.com, ojeda@...nel.org
Subject: [RFC char-misc-next 2/2] pps: Fix use-after-free cdev bug on module unload

Previously, a use-after-free KASAN splat could be reliably triggered
with:

    # insmod ./pps-ktimer.ko
    # rmmod pps-ktimer.ko
    <boom>

and CONFIG_DEBUG_KOBJECT_RELEASE=y.

This commit moves the driver to use cdev_alloc() instead of cdev_init()
to decouple the lifetime of struct cdev from struct pps_device.

We also make use of the previous commit's new cdev->private field to
store a pointer to the containing struct. We have to do this because
container_of() does not work when we store a pointer to struct cdev.

Signed-off-by: Daniel Xu <dxu@...uu.xyz>
---
 drivers/pps/pps.c          | 20 +++++++++++---------
 include/linux/pps_kernel.h |  2 +-
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 22a65ad4e46e..97ce26f67806 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -298,8 +298,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
 
 static int pps_cdev_open(struct inode *inode, struct file *file)
 {
-	struct pps_device *pps = container_of(inode->i_cdev,
-						struct pps_device, cdev);
+	struct pps_device *pps = inode->i_cdev->private;
 	file->private_data = pps;
 	kobject_get(&pps->dev->kobj);
 	return 0;
@@ -307,8 +306,7 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 
 static int pps_cdev_release(struct inode *inode, struct file *file)
 {
-	struct pps_device *pps = container_of(inode->i_cdev,
-						struct pps_device, cdev);
+	struct pps_device *pps = inode->i_cdev->private;
 	kobject_put(&pps->dev->kobj);
 	return 0;
 }
@@ -332,7 +330,7 @@ static void pps_device_destruct(struct device *dev)
 {
 	struct pps_device *pps = dev_get_drvdata(dev);
 
-	cdev_del(&pps->cdev);
+	cdev_del(pps->cdev);
 
 	/* Now we can release the ID for re-use */
 	pr_debug("deallocating pps%d\n", pps->id);
@@ -368,10 +366,14 @@ int pps_register_cdev(struct pps_device *pps)
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
-	cdev_init(&pps->cdev, &pps_cdev_fops);
-	pps->cdev.owner = pps->info.owner;
+	pps->cdev = cdev_alloc();
+	if (!pps->cdev)
+		goto free_idr;
+	pps->cdev->owner = pps->info.owner;
+	pps->cdev->ops = &pps_cdev_fops;
+	pps->cdev->private = pps;
 
-	err = cdev_add(&pps->cdev, devt, 1);
+	err = cdev_add(pps->cdev, devt, 1);
 	if (err) {
 		pr_err("%s: failed to add char device %d:%d\n",
 				pps->info.name, MAJOR(pps_devt), pps->id);
@@ -393,7 +395,7 @@ int pps_register_cdev(struct pps_device *pps)
 	return 0;
 
 del_cdev:
-	cdev_del(&pps->cdev);
+	cdev_del(pps->cdev);
 
 free_idr:
 	mutex_lock(&pps_idr_lock);
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 78c8ac4951b5..4e401793880f 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -56,7 +56,7 @@ struct pps_device {
 
 	unsigned int id;			/* PPS source unique ID */
 	void const *lookup_cookie;		/* For pps_lookup_dev() only */
-	struct cdev cdev;
+	struct cdev *cdev;
 	struct device *dev;
 	struct fasync_struct *async_queue;	/* fasync method */
 	spinlock_t lock;
-- 
2.34.1