[PATCH 0/6] pps: fix a UAF and clean up code

[PATCH 0/6] pps: fix a UAF and clean up code

[Michal Schmidt]

The 1st patch fixes a UAF bug. The fix is quite minimal, although a bit
ugly. The rest of the series are cleanups in the area.

Michal Schmidt (6):
  pps: fix cdev use-after-free
  pps: simplify pps_idr_lock locking
  pps: use scoped_guard for pps_idr_lock
  pps: print error in both cdev and dev error paths in
    pps_register_cdev()
  pps: embed "dev" in the pps_device
  pps: use cdev_device_add()

 drivers/pps/clients/pps-gpio.c    |  2 +-
 drivers/pps/clients/pps-ldisc.c   |  6 +-
 drivers/pps/clients/pps_parport.c |  4 +-
 drivers/pps/kapi.c                | 10 ++--
 drivers/pps/pps.c                 | 94 +++++++++++++------------------
 include/linux/pps_kernel.h        |  2 +-
 6 files changed, 50 insertions(+), 68 deletions(-)


base-commit: 7af08b57bcb9ebf78675c50069c54125c0a8b795
-- 
2.47.0

[PATCH 1/6] pps: fix cdev use-after-free

[Michal Schmidt]

The lifetime of a struct pps_device and the struct cdev embedded in it
is under control of the associated struct device.
The cdev's .open/.release methods (pps_cdev_{open,release}()) try to
keep the device alive while the cdev is open, but this is insufficient.

Consider this sequence:
1. Attach the PPS line discipline to a TTY.
2. Open the created /dev/pps* file.
3. Detach the PPS line discipline.
4. Close the file.

In this scenario, the last reference to the cdev is not released from
pps code, but in __fput(), *after* running the .release method, which
frees the pps_device (including struct cdev).

Fix this by using cdev_set_parent() to ensure that the pps_device
outlives the cdev.

cdev_set_parent() should be used before cdev_add(), but we can't do that
here, because at that point we don't have the dev yet. It's created
in the next step with device_create(). To compensate, bump the refcount
of the dev, which is what cdev_add() would have done if we followed the
usual order. This will be cleaned up in subsequent patches.

With the parent relationship in place, the .open/.release methods no
longer need to change the refcount. The cdev reference held by the core
filesystem code is enough to keep the pps device alive.
The .release method had nothing else to do, so remove it.

Move the cdev_del() from pps_device_destruct() to pps_unregister_cdev().
This is necessary. Otherwise, the pps_device would be holding a
reference to itself and never get released. It also brings symmetry
between pps_register_cdev() and pps_unregister_cdev().

KASAN detection of the bug:

 pps_core: deallocating pps0
 ==================================================================
 BUG: KASAN: slab-use-after-free in cdev_put+0x4e/0x50
 Read of size 8 at addr ff1100001c1c7360 by task sleep/1192

 CPU: 0 UID: 0 PID: 1192 Comm: sleep Not tainted 6.12.0-0.rc7.59.fc42.x86_64+debug #1
 Hardware name: Red Hat OpenStack Compute, BIOS 1.14.0-1.module+el8.4.0+8855+a9e237a9 04/01/2014
 Call Trace:
  <TASK>
  dump_stack_lvl+0x84/0xd0
  print_report+0x174/0x505
  kasan_report+0xab/0x180
  cdev_put+0x4e/0x50
  __fput+0x725/0xaa0
  task_work_run+0x119/0x200
  do_exit+0x8ef/0x27a0
  do_group_exit+0xbc/0x250
  get_signal+0x1b78/0x1e00
  arch_do_signal_or_restart+0x8f/0x570
  syscall_exit_to_user_mode+0x1f4/0x290
  do_syscall_64+0xa3/0x190
  entry_SYSCALL_64_after_hwframe+0x76/0x7e
 RIP: 0033:0x7f6c598342d6
 Code: Unable to access opcode bytes at 0x7f6c598342ac.
 RSP: 002b:00007ffff3528160 EFLAGS: 00000202 ORIG_RAX: 00000000000000e6
 RAX: fffffffffffffdfc RBX: 00007f6c597c5740 RCX: 00007f6c598342d6
 RDX: 00007ffff35281f0 RSI: 0000000000000000 RDI: 0000000000000000
 RBP: 00007ffff3528170 R08: 0000000000000000 R09: 0000000000000000
 R10: 00007ffff35281e0 R11: 0000000000000202 R12: 00007f6c597c56c8
 R13: 00007ffff35281e0 R14: 000000000000003c R15: 00007ffff35281e0
  </TASK>

 Allocated by task 1186:
  kasan_save_stack+0x30/0x50
  kasan_save_track+0x14/0x30
  __kasan_kmalloc+0x8f/0xa0
  pps_register_source+0xe4/0x360
  pps_tty_open+0x191/0x220 [pps_ldisc]
  tty_ldisc_open+0x75/0xc0
  tty_set_ldisc+0x29e/0x730
  tty_ioctl+0x866/0x11e0
  __x64_sys_ioctl+0x12e/0x1a0
  do_syscall_64+0x97/0x190
  entry_SYSCALL_64_after_hwframe+0x76/0x7e

 Freed by task 1192:
  kasan_save_stack+0x30/0x50
  kasan_save_track+0x14/0x30
  kasan_save_free_info+0x3b/0x70
  __kasan_slab_free+0x37/0x50
  kfree+0x140/0x4d0
  device_release+0x9c/0x210
  kobject_put+0x17c/0x4b0
  pps_cdev_release+0x56/0x70
  __fput+0x368/0xaa0
  task_work_run+0x119/0x200
  do_exit+0x8ef/0x27a0
  do_group_exit+0xbc/0x250
  get_signal+0x1b78/0x1e00
  arch_do_signal_or_restart+0x8f/0x570
  syscall_exit_to_user_mode+0x1f4/0x290
  do_syscall_64+0xa3/0x190
  entry_SYSCALL_64_after_hwframe+0x76/0x7e

 The buggy address belongs to the object at ff1100001c1c7200
                which belongs to the cache kmalloc-rnd-02-512 of size 512
 The buggy address is located 352 bytes inside of
                freed 512-byte region [ff1100001c1c7200, ff1100001c1c7400)
 [...]
 ==================================================================

Fixes: eae9d2ba0cfc ("LinuxPPS: core support")
Fixes: d953e0e837e6 ("pps: Fix a use-after free bug when unregistering a source.")
Cc: stable@vger.kernel.org
Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/pps.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 25d47907db17..4f497353daa2 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -301,15 +301,6 @@ static int pps_cdev_open(struct inode *inode, struct file *file)
 	struct pps_device *pps = container_of(inode->i_cdev,
 						struct pps_device, cdev);
 	file->private_data = pps;
-	kobject_get(&pps->dev->kobj);
-	return 0;
-}
-
-static int pps_cdev_release(struct inode *inode, struct file *file)
-{
-	struct pps_device *pps = container_of(inode->i_cdev,
-						struct pps_device, cdev);
-	kobject_put(&pps->dev->kobj);
 	return 0;
 }
 
@@ -324,15 +315,12 @@ static const struct file_operations pps_cdev_fops = {
 	.compat_ioctl	= pps_cdev_compat_ioctl,
 	.unlocked_ioctl	= pps_cdev_ioctl,
 	.open		= pps_cdev_open,
-	.release	= pps_cdev_release,
 };
 
 static void pps_device_destruct(struct device *dev)
 {
 	struct pps_device *pps = dev_get_drvdata(dev);
 
-	cdev_del(&pps->cdev);
-
 	/* Now we can release the ID for re-use */
 	pr_debug("deallocating pps%d\n", pps->id);
 	mutex_lock(&pps_idr_lock);
@@ -383,6 +371,10 @@ int pps_register_cdev(struct pps_device *pps)
 		goto del_cdev;
 	}
 
+	cdev_set_parent(&pps->cdev, &pps->dev->kobj);
+	/* Compensate for setting the parent after cdev_add() */
+	get_device(pps->dev);
+
 	/* Override the release function with our own */
 	pps->dev->release = pps_device_destruct;
 
@@ -407,6 +399,7 @@ void pps_unregister_cdev(struct pps_device *pps)
 	pr_debug("unregistering pps%d\n", pps->id);
 	pps->lookup_cookie = NULL;
 	device_destroy(pps_class, pps->dev->devt);
+	cdev_del(&pps->cdev);
 }
 
 /*
-- 
2.47.0

[PATCH 2/6] pps: simplify pps_idr_lock locking

[Michal Schmidt]

It is sufficient to hold pps_idr_lock only around the idr
(de)allocations. There is no reason for the assignment to pps->id or for
the error printing to be under the lock.

Simplify the pps_idr_lock locking and the error path.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/pps.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 4f497353daa2..9a631f6d3a1e 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -336,22 +336,22 @@ int pps_register_cdev(struct pps_device *pps)
 	int err;
 	dev_t devt;
 
-	mutex_lock(&pps_idr_lock);
 	/*
 	 * Get new ID for the new PPS source.  After idr_alloc() calling
 	 * the new source will be freely available into the kernel.
 	 */
+	mutex_lock(&pps_idr_lock);
 	err = idr_alloc(&pps_idr, pps, 0, PPS_MAX_SOURCES, GFP_KERNEL);
+	mutex_unlock(&pps_idr_lock);
 	if (err < 0) {
 		if (err == -ENOSPC) {
 			pr_err("%s: too many PPS sources in the system\n",
 			       pps->info.name);
 			err = -EBUSY;
 		}
-		goto out_unlock;
+		return err;
 	}
 	pps->id = err;
-	mutex_unlock(&pps_idr_lock);
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
@@ -389,7 +389,6 @@ int pps_register_cdev(struct pps_device *pps)
 free_idr:
 	mutex_lock(&pps_idr_lock);
 	idr_remove(&pps_idr, pps->id);
-out_unlock:
 	mutex_unlock(&pps_idr_lock);
 	return err;
 }
-- 
2.47.0

[PATCH 3/6] pps: use scoped_guard for pps_idr_lock

[Michal Schmidt]

Use the scoped_guard syntax to be more brief.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/pps.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 9a631f6d3a1e..92be7815a621 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -323,9 +323,8 @@ static void pps_device_destruct(struct device *dev)
 
 	/* Now we can release the ID for re-use */
 	pr_debug("deallocating pps%d\n", pps->id);
-	mutex_lock(&pps_idr_lock);
-	idr_remove(&pps_idr, pps->id);
-	mutex_unlock(&pps_idr_lock);
+	scoped_guard(mutex, &pps_idr_lock)
+		idr_remove(&pps_idr, pps->id);
 
 	kfree(dev);
 	kfree(pps);
@@ -340,9 +339,8 @@ int pps_register_cdev(struct pps_device *pps)
 	 * Get new ID for the new PPS source.  After idr_alloc() calling
 	 * the new source will be freely available into the kernel.
 	 */
-	mutex_lock(&pps_idr_lock);
-	err = idr_alloc(&pps_idr, pps, 0, PPS_MAX_SOURCES, GFP_KERNEL);
-	mutex_unlock(&pps_idr_lock);
+	scoped_guard(mutex, &pps_idr_lock)
+		err = idr_alloc(&pps_idr, pps, 0, PPS_MAX_SOURCES, GFP_KERNEL);
 	if (err < 0) {
 		if (err == -ENOSPC) {
 			pr_err("%s: too many PPS sources in the system\n",
@@ -387,9 +385,8 @@ int pps_register_cdev(struct pps_device *pps)
 	cdev_del(&pps->cdev);
 
 free_idr:
-	mutex_lock(&pps_idr_lock);
-	idr_remove(&pps_idr, pps->id);
-	mutex_unlock(&pps_idr_lock);
+	scoped_guard(mutex, &pps_idr_lock)
+		idr_remove(&pps_idr, pps->id);
 	return err;
 }
 
-- 
2.47.0

Re: [PATCH 3/6] pps: use scoped_guard for pps_idr_lock

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 269 bytes --]

Hello,

On Mon, Dec 02, 2024 at 05:34:48PM +0100, Michal Schmidt wrote:
> Use the scoped_guard syntax to be more brief.
> 
> Signed-off-by: Michal Schmidt <mschmidt@redhat.com>

This patch undoes most of patch 2. I'd squash these together.

Best regards
Uwe

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH 4/6] pps: print error in both cdev and dev error paths in pps_register_cdev()

[Michal Schmidt]

For pps, a device_create() failure is just as severe as a cdev_add()
failure. Print the error message in both cases.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/pps.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 92be7815a621..f90ee483d343 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -357,11 +357,9 @@ int pps_register_cdev(struct pps_device *pps)
 	pps->cdev.owner = pps->info.owner;
 
 	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);
+	if (err)
 		goto free_idr;
-	}
+
 	pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
 							"pps%d", pps->id);
 	if (IS_ERR(pps->dev)) {
@@ -387,6 +385,8 @@ int pps_register_cdev(struct pps_device *pps)
 free_idr:
 	scoped_guard(mutex, &pps_idr_lock)
 		idr_remove(&pps_idr, pps->id);
+	pr_err("%s: failed to add char device %d:%d\n",
+	       pps->info.name, MAJOR(pps_devt), pps->id);
 	return err;
 }
 
-- 
2.47.0

[PATCH 5/6] pps: embed "dev" in the pps_device

[Michal Schmidt]

Embed the struct device directly in struct pps_device.
Use device_initialize() + device_add() instead of device_create().

Advantages:
  Avoids a separate allocation.
  Lets us set dev as cdev's parent without the extra get_device() hack.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/clients/pps-gpio.c    |  2 +-
 drivers/pps/clients/pps-ldisc.c   |  6 +--
 drivers/pps/clients/pps_parport.c |  4 +-
 drivers/pps/kapi.c                | 10 ++---
 drivers/pps/pps.c                 | 61 +++++++++++++++----------------
 include/linux/pps_kernel.h        |  2 +-
 6 files changed, 42 insertions(+), 43 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 791fdc9326dd..a21bf56e7a87 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -214,7 +214,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	dev_info(data->pps->dev, "Registered IRQ %d as PPS source\n",
+	dev_info(&data->pps->dev, "Registered IRQ %d as PPS source\n",
 		 data->irq);
 
 	return 0;
diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index 443d6bae19d1..49ae33cca03d 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -32,7 +32,7 @@ static void pps_tty_dcd_change(struct tty_struct *tty, bool active)
 	pps_event(pps, &ts, active ? PPS_CAPTUREASSERT :
 			PPS_CAPTURECLEAR, NULL);
 
-	dev_dbg(pps->dev, "PPS %s at %lu\n",
+	dev_dbg(&pps->dev, "PPS %s at %lu\n",
 			active ? "assert" : "clear", jiffies);
 }
 
@@ -69,7 +69,7 @@ static int pps_tty_open(struct tty_struct *tty)
 		goto err_unregister;
 	}
 
-	dev_info(pps->dev, "source \"%s\" added\n", info.path);
+	dev_info(&pps->dev, "source \"%s\" added\n", info.path);
 
 	return 0;
 
@@ -89,7 +89,7 @@ static void pps_tty_close(struct tty_struct *tty)
 	if (WARN_ON(!pps))
 		return;
 
-	dev_info(pps->dev, "removed\n");
+	dev_info(&pps->dev, "removed\n");
 	pps_unregister_source(pps);
 }
 
diff --git a/drivers/pps/clients/pps_parport.c b/drivers/pps/clients/pps_parport.c
index abaffb4e1c1c..24db06750297 100644
--- a/drivers/pps/clients/pps_parport.c
+++ b/drivers/pps/clients/pps_parport.c
@@ -81,7 +81,7 @@ static void parport_irq(void *handle)
 	/* check the signal (no signal means the pulse is lost this time) */
 	if (!signal_is_set(port)) {
 		local_irq_restore(flags);
-		dev_err(dev->pps->dev, "lost the signal\n");
+		dev_err(&dev->pps->dev, "lost the signal\n");
 		goto out_assert;
 	}
 
@@ -98,7 +98,7 @@ static void parport_irq(void *handle)
 	/* timeout */
 	dev->cw_err++;
 	if (dev->cw_err >= CLEAR_WAIT_MAX_ERRORS) {
-		dev_err(dev->pps->dev, "disabled clear edge capture after %d"
+		dev_err(&dev->pps->dev, "disabled clear edge capture after %d"
 				" timeouts\n", dev->cw_err);
 		dev->cw = 0;
 		dev->cw_err = 0;
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index d9d566f70ed1..a76685c147ee 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -41,7 +41,7 @@ static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
 static void pps_echo_client_default(struct pps_device *pps, int event,
 		void *data)
 {
-	dev_info(pps->dev, "echo %s %s\n",
+	dev_info(&pps->dev, "echo %s %s\n",
 		event & PPS_CAPTUREASSERT ? "assert" : "",
 		event & PPS_CAPTURECLEAR ? "clear" : "");
 }
@@ -112,7 +112,7 @@ struct pps_device *pps_register_source(struct pps_source_info *info,
 		goto kfree_pps;
 	}
 
-	dev_info(pps->dev, "new PPS source %s\n", info->name);
+	dev_info(&pps->dev, "new PPS source %s\n", info->name);
 
 	return pps;
 
@@ -166,7 +166,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	/* check event type */
 	BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
 
-	dev_dbg(pps->dev, "PPS event at %lld.%09ld\n",
+	dev_dbg(&pps->dev, "PPS event at %lld.%09ld\n",
 			(s64)ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
 
 	timespec_to_pps_ktime(&ts_real, ts->ts_real);
@@ -188,7 +188,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 		/* Save the time stamp */
 		pps->assert_tu = ts_real;
 		pps->assert_sequence++;
-		dev_dbg(pps->dev, "capture assert seq #%u\n",
+		dev_dbg(&pps->dev, "capture assert seq #%u\n",
 			pps->assert_sequence);
 
 		captured = ~0;
@@ -202,7 +202,7 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 		/* Save the time stamp */
 		pps->clear_tu = ts_real;
 		pps->clear_sequence++;
-		dev_dbg(pps->dev, "capture clear seq #%u\n",
+		dev_dbg(&pps->dev, "capture clear seq #%u\n",
 			pps->clear_sequence);
 
 		captured = ~0;
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index f90ee483d343..64a8b34aa4ce 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -62,7 +62,7 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
 	else {
 		unsigned long ticks;
 
-		dev_dbg(pps->dev, "timeout %lld.%09d\n",
+		dev_dbg(&pps->dev, "timeout %lld.%09d\n",
 				(long long) fdata->timeout.sec,
 				fdata->timeout.nsec);
 		ticks = fdata->timeout.sec * HZ;
@@ -80,7 +80,7 @@ static int pps_cdev_pps_fetch(struct pps_device *pps, struct pps_fdata *fdata)
 
 	/* Check for pending signals */
 	if (err == -ERESTARTSYS) {
-		dev_dbg(pps->dev, "pending signal caught\n");
+		dev_dbg(&pps->dev, "pending signal caught\n");
 		return -EINTR;
 	}
 
@@ -98,7 +98,7 @@ static long pps_cdev_ioctl(struct file *file,
 
 	switch (cmd) {
 	case PPS_GETPARAMS:
-		dev_dbg(pps->dev, "PPS_GETPARAMS\n");
+		dev_dbg(&pps->dev, "PPS_GETPARAMS\n");
 
 		spin_lock_irq(&pps->lock);
 
@@ -114,7 +114,7 @@ static long pps_cdev_ioctl(struct file *file,
 		break;
 
 	case PPS_SETPARAMS:
-		dev_dbg(pps->dev, "PPS_SETPARAMS\n");
+		dev_dbg(&pps->dev, "PPS_SETPARAMS\n");
 
 		/* Check the capabilities */
 		if (!capable(CAP_SYS_TIME))
@@ -124,14 +124,14 @@ static long pps_cdev_ioctl(struct file *file,
 		if (err)
 			return -EFAULT;
 		if (!(params.mode & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR))) {
-			dev_dbg(pps->dev, "capture mode unspecified (%x)\n",
+			dev_dbg(&pps->dev, "capture mode unspecified (%x)\n",
 								params.mode);
 			return -EINVAL;
 		}
 
 		/* Check for supported capabilities */
 		if ((params.mode & ~pps->info.mode) != 0) {
-			dev_dbg(pps->dev, "unsupported capabilities (%x)\n",
+			dev_dbg(&pps->dev, "unsupported capabilities (%x)\n",
 								params.mode);
 			return -EINVAL;
 		}
@@ -144,7 +144,7 @@ static long pps_cdev_ioctl(struct file *file,
 		/* Restore the read only parameters */
 		if ((params.mode & (PPS_TSFMT_TSPEC | PPS_TSFMT_NTPFP)) == 0) {
 			/* section 3.3 of RFC 2783 interpreted */
-			dev_dbg(pps->dev, "time format unspecified (%x)\n",
+			dev_dbg(&pps->dev, "time format unspecified (%x)\n",
 								params.mode);
 			pps->params.mode |= PPS_TSFMT_TSPEC;
 		}
@@ -165,7 +165,7 @@ static long pps_cdev_ioctl(struct file *file,
 		break;
 
 	case PPS_GETCAP:
-		dev_dbg(pps->dev, "PPS_GETCAP\n");
+		dev_dbg(&pps->dev, "PPS_GETCAP\n");
 
 		err = put_user(pps->info.mode, iuarg);
 		if (err)
@@ -176,7 +176,7 @@ static long pps_cdev_ioctl(struct file *file,
 	case PPS_FETCH: {
 		struct pps_fdata fdata;
 
-		dev_dbg(pps->dev, "PPS_FETCH\n");
+		dev_dbg(&pps->dev, "PPS_FETCH\n");
 
 		err = copy_from_user(&fdata, uarg, sizeof(struct pps_fdata));
 		if (err)
@@ -206,7 +206,7 @@ static long pps_cdev_ioctl(struct file *file,
 	case PPS_KC_BIND: {
 		struct pps_bind_args bind_args;
 
-		dev_dbg(pps->dev, "PPS_KC_BIND\n");
+		dev_dbg(&pps->dev, "PPS_KC_BIND\n");
 
 		/* Check the capabilities */
 		if (!capable(CAP_SYS_TIME))
@@ -218,7 +218,7 @@ static long pps_cdev_ioctl(struct file *file,
 
 		/* Check for supported capabilities */
 		if ((bind_args.edge & ~pps->info.mode) != 0) {
-			dev_err(pps->dev, "unsupported capabilities (%x)\n",
+			dev_err(&pps->dev, "unsupported capabilities (%x)\n",
 					bind_args.edge);
 			return -EINVAL;
 		}
@@ -227,7 +227,7 @@ static long pps_cdev_ioctl(struct file *file,
 		if (bind_args.tsformat != PPS_TSFMT_TSPEC ||
 				(bind_args.edge & ~PPS_CAPTUREBOTH) != 0 ||
 				bind_args.consumer != PPS_KC_HARDPPS) {
-			dev_err(pps->dev, "invalid kernel consumer bind"
+			dev_err(&pps->dev, "invalid kernel consumer bind"
 					" parameters (%x)\n", bind_args.edge);
 			return -EINVAL;
 		}
@@ -259,7 +259,7 @@ static long pps_cdev_compat_ioctl(struct file *file,
 		struct pps_fdata fdata;
 		int err;
 
-		dev_dbg(pps->dev, "PPS_FETCH\n");
+		dev_dbg(&pps->dev, "PPS_FETCH\n");
 
 		err = copy_from_user(&compat, uarg, sizeof(struct pps_fdata_compat));
 		if (err)
@@ -319,14 +319,13 @@ static const struct file_operations pps_cdev_fops = {
 
 static void pps_device_destruct(struct device *dev)
 {
-	struct pps_device *pps = dev_get_drvdata(dev);
+	struct pps_device *pps = container_of(dev, struct pps_device, dev);
 
 	/* Now we can release the ID for re-use */
 	pr_debug("deallocating pps%d\n", pps->id);
 	scoped_guard(mutex, &pps_idr_lock)
 		idr_remove(&pps_idr, pps->id);
 
-	kfree(dev);
 	kfree(pps);
 }
 
@@ -353,26 +352,25 @@ int pps_register_cdev(struct pps_device *pps)
 
 	devt = MKDEV(MAJOR(pps_devt), pps->id);
 
+	device_initialize(&pps->dev);
+	pps->dev.devt = devt;
+	pps->dev.class = pps_class;
+	pps->dev.parent = pps->info.dev;
+	err = dev_set_name(&pps->dev, "pps%d", pps->id);
+	if (err)
+		goto put_dev;
 	cdev_init(&pps->cdev, &pps_cdev_fops);
 	pps->cdev.owner = pps->info.owner;
-
+	cdev_set_parent(&pps->cdev, &pps->dev.kobj);
 	err = cdev_add(&pps->cdev, devt, 1);
 	if (err)
-		goto free_idr;
-
-	pps->dev = device_create(pps_class, pps->info.dev, devt, pps,
-							"pps%d", pps->id);
-	if (IS_ERR(pps->dev)) {
-		err = PTR_ERR(pps->dev);
+		goto put_dev;
+	err = device_add(&pps->dev);
+	if (err)
 		goto del_cdev;
-	}
-
-	cdev_set_parent(&pps->cdev, &pps->dev->kobj);
-	/* Compensate for setting the parent after cdev_add() */
-	get_device(pps->dev);
 
 	/* Override the release function with our own */
-	pps->dev->release = pps_device_destruct;
+	pps->dev.release = pps_device_destruct;
 
 	pr_debug("source %s got cdev (%d:%d)\n", pps->info.name,
 			MAJOR(pps_devt), pps->id);
@@ -381,8 +379,8 @@ int pps_register_cdev(struct pps_device *pps)
 
 del_cdev:
 	cdev_del(&pps->cdev);
-
-free_idr:
+put_dev:
+	put_device(&pps->dev);
 	scoped_guard(mutex, &pps_idr_lock)
 		idr_remove(&pps_idr, pps->id);
 	pr_err("%s: failed to add char device %d:%d\n",
@@ -394,8 +392,9 @@ void pps_unregister_cdev(struct pps_device *pps)
 {
 	pr_debug("unregistering pps%d\n", pps->id);
 	pps->lookup_cookie = NULL;
-	device_destroy(pps_class, pps->dev->devt);
+	device_del(&pps->dev);
 	cdev_del(&pps->cdev);
+	put_device(&pps->dev);
 }
 
 /*
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 78c8ac4951b5..d1edacb7e63e 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -57,7 +57,7 @@ struct pps_device {
 	unsigned int id;			/* PPS source unique ID */
 	void const *lookup_cookie;		/* For pps_lookup_dev() only */
 	struct cdev cdev;
-	struct device *dev;
+	struct device dev;
 	struct fasync_struct *async_queue;	/* fasync method */
 	spinlock_t lock;
 };
-- 
2.47.0

Re: [PATCH 5/6] pps: embed "dev" in the pps_device

[kernel test robot]

Hi Michal,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7af08b57bcb9ebf78675c50069c54125c0a8b795]

url:    https://github.com/intel-lab-lkp/linux/commits/Michal-Schmidt/pps-fix-cdev-use-after-free/20241203-025037
base:   7af08b57bcb9ebf78675c50069c54125c0a8b795
patch link:    https://lore.kernel.org/r/20241202163451.1442566-6-mschmidt%40redhat.com
patch subject: [PATCH 5/6] pps: embed "dev" in the pps_device
config: arm-randconfig-002-20241203 (https://download.01.org/0day-ci/archive/20241203/202412030824.tmsDhCFT-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412030824.tmsDhCFT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412030824.tmsDhCFT-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/linux/device.h:15,
                    from include/linux/cdev.h:8,
                    from include/linux/pps_kernel.h:12,
                    from drivers/pps/clients/pps-ktimer.c:15:
   drivers/pps/clients/pps-ktimer.c: In function 'pps_ktimer_exit':
>> drivers/pps/clients/pps-ktimer.c:59:21: error: incompatible type for argument 1 of '_dev_info'
      59 |         dev_info(pps->dev, "ktimer PPS source unregistered\n");
         |                  ~~~^~~~~
         |                     |
         |                     struct device
   include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   drivers/pps/clients/pps-ktimer.c:59:9: note: in expansion of macro 'dev_info'
      59 |         dev_info(pps->dev, "ktimer PPS source unregistered\n");
         |         ^~~~~~~~
   include/linux/dev_printk.h:95:37: note: expected 'const struct device *' but argument is of type 'struct device'
      95 | void _dev_info(const struct device *dev, const char *fmt, ...)
         |                ~~~~~~~~~~~~~~~~~~~~~^~~
   drivers/pps/clients/pps-ktimer.c: In function 'pps_ktimer_init':
   drivers/pps/clients/pps-ktimer.c:77:21: error: incompatible type for argument 1 of '_dev_info'
      77 |         dev_info(pps->dev, "ktimer PPS source registered\n");
         |                  ~~~^~~~~
         |                     |
         |                     struct device
   include/linux/dev_printk.h:110:25: note: in definition of macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   drivers/pps/clients/pps-ktimer.c:77:9: note: in expansion of macro 'dev_info'
      77 |         dev_info(pps->dev, "ktimer PPS source registered\n");
         |         ^~~~~~~~
   include/linux/dev_printk.h:95:37: note: expected 'const struct device *' but argument is of type 'struct device'
      95 | void _dev_info(const struct device *dev, const char *fmt, ...)
         |                ~~~~~~~~~~~~~~~~~~~~~^~~


vim +/_dev_info +59 drivers/pps/clients/pps-ktimer.c

697fb85fcf21b5 Rodolfo Giometti  2010-03-10  52  
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  53  /*
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  54   * Module staff
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  55   */
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  56  
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  57  static void __exit pps_ktimer_exit(void)
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  58  {
5e196d34a77642 Alexander Gordeev 2011-01-12 @59  	dev_info(pps->dev, "ktimer PPS source unregistered\n");
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  60  
5e196d34a77642 Alexander Gordeev 2011-01-12  61  	del_timer_sync(&ktimer);
5e196d34a77642 Alexander Gordeev 2011-01-12  62  	pps_unregister_source(pps);
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  63  }
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  64  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 5/6] pps: embed "dev" in the pps_device

[kernel test robot]

Hi Michal,

kernel test robot noticed the following build errors:

[auto build test ERROR on 7af08b57bcb9ebf78675c50069c54125c0a8b795]

url:    https://github.com/intel-lab-lkp/linux/commits/Michal-Schmidt/pps-fix-cdev-use-after-free/20241203-025037
base:   7af08b57bcb9ebf78675c50069c54125c0a8b795
patch link:    https://lore.kernel.org/r/20241202163451.1442566-6-mschmidt%40redhat.com
patch subject: [PATCH 5/6] pps: embed "dev" in the pps_device
config: i386-buildonly-randconfig-005-20241203 (https://download.01.org/0day-ci/archive/20241203/202412030942.aHisJx3i-lkp@intel.com/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241203/202412030942.aHisJx3i-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202412030942.aHisJx3i-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/pps/clients/pps-ktimer.c:59:11: error: passing 'struct device' to parameter of incompatible type 'const struct device *'; take the address with &
      59 |         dev_info(pps->dev, "ktimer PPS source unregistered\n");
         |                  ^~~~~~~~
         |                  &
   include/linux/dev_printk.h:160:46: note: expanded from macro 'dev_info'
     160 |         dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                     ^~~
   include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   include/linux/dev_printk.h:56:37: note: passing argument to parameter 'dev' here
      56 | void _dev_info(const struct device *dev, const char *fmt, ...);
         |                                     ^
   drivers/pps/clients/pps-ktimer.c:77:11: error: passing 'struct device' to parameter of incompatible type 'const struct device *'; take the address with &
      77 |         dev_info(pps->dev, "ktimer PPS source registered\n");
         |                  ^~~~~~~~
         |                  &
   include/linux/dev_printk.h:160:46: note: expanded from macro 'dev_info'
     160 |         dev_printk_index_wrap(_dev_info, KERN_INFO, dev, dev_fmt(fmt), ##__VA_ARGS__)
         |                                                     ^~~
   include/linux/dev_printk.h:110:11: note: expanded from macro 'dev_printk_index_wrap'
     110 |                 _p_func(dev, fmt, ##__VA_ARGS__);                       \
         |                         ^~~
   include/linux/dev_printk.h:56:37: note: passing argument to parameter 'dev' here
      56 | void _dev_info(const struct device *dev, const char *fmt, ...);
         |                                     ^
   2 errors generated.


vim +59 drivers/pps/clients/pps-ktimer.c

697fb85fcf21b5 Rodolfo Giometti  2010-03-10  52  
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  53  /*
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  54   * Module staff
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  55   */
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  56  
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  57  static void __exit pps_ktimer_exit(void)
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  58  {
5e196d34a77642 Alexander Gordeev 2011-01-12 @59  	dev_info(pps->dev, "ktimer PPS source unregistered\n");
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  60  
5e196d34a77642 Alexander Gordeev 2011-01-12  61  	del_timer_sync(&ktimer);
5e196d34a77642 Alexander Gordeev 2011-01-12  62  	pps_unregister_source(pps);
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  63  }
697fb85fcf21b5 Rodolfo Giometti  2010-03-10  64  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 6/6] pps: use cdev_device_add()

[Michal Schmidt]

The cdev_set_parent() + cdev_add() + device_add() sequence is just
open-coded cdev_device_add(). Use it.

Signed-off-by: Michal Schmidt <mschmidt@redhat.com>
---
 drivers/pps/pps.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 64a8b34aa4ce..89b502855e7b 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -361,13 +361,10 @@ int pps_register_cdev(struct pps_device *pps)
 		goto put_dev;
 	cdev_init(&pps->cdev, &pps_cdev_fops);
 	pps->cdev.owner = pps->info.owner;
-	cdev_set_parent(&pps->cdev, &pps->dev.kobj);
-	err = cdev_add(&pps->cdev, devt, 1);
+
+	err = cdev_device_add(&pps->cdev, &pps->dev);
 	if (err)
 		goto put_dev;
-	err = device_add(&pps->dev);
-	if (err)
-		goto del_cdev;
 
 	/* Override the release function with our own */
 	pps->dev.release = pps_device_destruct;
@@ -377,8 +374,6 @@ int pps_register_cdev(struct pps_device *pps)
 
 	return 0;
 
-del_cdev:
-	cdev_del(&pps->cdev);
 put_dev:
 	put_device(&pps->dev);
 	scoped_guard(mutex, &pps_idr_lock)
@@ -392,8 +387,7 @@ void pps_unregister_cdev(struct pps_device *pps)
 {
 	pr_debug("unregistering pps%d\n", pps->id);
 	pps->lookup_cookie = NULL;
-	device_del(&pps->dev);
-	cdev_del(&pps->cdev);
+	cdev_device_del(&pps->cdev, &pps->dev);
 	put_device(&pps->dev);
 }
 
-- 
2.47.0

Re: [PATCH 0/6] pps: fix a UAF and clean up code

[Calvin Owens]

On Monday 12/02 at 17:34 +0100, Michal Schmidt wrote:
> The 1st patch fixes a UAF bug. The fix is quite minimal, although a bit
> ugly. The rest of the series are cleanups in the area.

Hi Michal,

I've also been poking at this, I've sent some patches recently:

    https://lore.kernel.org/all/a17975fd5ae99385791929e563f72564edbcf28f.1731383727.git.calvin@wbinvd.org/

The major difference is that I used __register_chrdev() and removed the
embedded cdev too. I think that is cleaner, what do you think?

Thanks,
Calvin

> Michal Schmidt (6):
>   pps: fix cdev use-after-free
>   pps: simplify pps_idr_lock locking
>   pps: use scoped_guard for pps_idr_lock
>   pps: print error in both cdev and dev error paths in
>     pps_register_cdev()
>   pps: embed "dev" in the pps_device
>   pps: use cdev_device_add()
> 
>  drivers/pps/clients/pps-gpio.c    |  2 +-
>  drivers/pps/clients/pps-ldisc.c   |  6 +-
>  drivers/pps/clients/pps_parport.c |  4 +-
>  drivers/pps/kapi.c                | 10 ++--
>  drivers/pps/pps.c                 | 94 +++++++++++++------------------
>  include/linux/pps_kernel.h        |  2 +-
>  6 files changed, 50 insertions(+), 68 deletions(-)
> 
> 
> base-commit: 7af08b57bcb9ebf78675c50069c54125c0a8b795
> -- 
> 2.47.0
> 

Re: [PATCH 0/6] pps: fix a UAF and clean up code

[Michal Schmidt]

On Mon, Dec 2, 2024 at 5:56 PM Calvin Owens <calvin@wbinvd.org> wrote:
>
> On Monday 12/02 at 17:34 +0100, Michal Schmidt wrote:
> > The 1st patch fixes a UAF bug. The fix is quite minimal, although a bit
> > ugly. The rest of the series are cleanups in the area.
>
> Hi Michal,
>
> I've also been poking at this, I've sent some patches recently:
>
>     https://lore.kernel.org/all/a17975fd5ae99385791929e563f72564edbcf28f.1731383727.git.calvin@wbinvd.org/
>
> The major difference is that I used __register_chrdev() and removed the
> embedded cdev too. I think that is cleaner, what do you think?
>
> Thanks,
> Calvin

I agree. Let's go with yours.
Michal

> > Michal Schmidt (6):
> >   pps: fix cdev use-after-free
> >   pps: simplify pps_idr_lock locking
> >   pps: use scoped_guard for pps_idr_lock
> >   pps: print error in both cdev and dev error paths in
> >     pps_register_cdev()
> >   pps: embed "dev" in the pps_device
> >   pps: use cdev_device_add()
> >
> >  drivers/pps/clients/pps-gpio.c    |  2 +-
> >  drivers/pps/clients/pps-ldisc.c   |  6 +-
> >  drivers/pps/clients/pps_parport.c |  4 +-
> >  drivers/pps/kapi.c                | 10 ++--
> >  drivers/pps/pps.c                 | 94 +++++++++++++------------------
> >  include/linux/pps_kernel.h        |  2 +-
> >  6 files changed, 50 insertions(+), 68 deletions(-)
> >
> >
> > base-commit: 7af08b57bcb9ebf78675c50069c54125c0a8b795
> > --
> > 2.47.0
> >
>