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); }
/*