Re: [PATCH v2 2/4] pps: generators: don't use the driver's info after unregister

[Rodolfo Giometti <giometti@enneenne.com>]

On Tue, 29 Sep 2026 07:49:35 -0500, Danish Khateeb wrote:
> A file that was opened before pps_gen_unregister_source() keeps
> pps_gen, and with it the pointer to the driver's pps_gen_source_info.
> PPS_GEN_SETENABLE and PPS_GEN_USESYSTEMCLOCK keep using that pointer
> after the driver has gone:
>
>  - pps_gen_tio allocates its info with devm_kzalloc(), so after an
>    unbind both ioctls read freed memory, and SETENABLE calls the
>    driver's enable() with its freed private data.
>
>  - pps_gen-dummy does not set info->owner, so it can be unloaded while
>    the file is open, and the ioctls then read the unloaded module's
>    data and call its code.
>
> TIO can't probe in a VM, as it needs ART. A test driver that registers
> its info in devm memory like TIO does, unbound while /dev/pps-gen0 is
> open, gives:
>
>   BUG: KASAN: slab-use-after-free in pps_gen_cdev_ioctl+0x4c1/0x580
>   Read of size 1 at addr ffff88800e647f38 by task ppsgen64/145
>   ...
>   Freed by task 146:
>    kfree+0x25a/0x6d0
>    release_nodes+0xd1/0x140
>    devres_release_all+0x10e/0x1a0
>    device_unbind_cleanup+0x71/0x250
>    device_release_driver_internal+0x41b/0x570
>    unbind_store+0xd9/0x100
>
> and unloading pps_gen-dummy while its file is open oopses:
>
>   BUG: unable to handle page fault for address: ffffffffa0203040
>   Oops: Oops: 0000 [#1] SMP KASAN NOPTI
>   RIP: 0010:pps_gen_cdev_ioctl+0x372/0x580
>
> Protect info in the ioctl handler with a mutex, and clear it under the
> mutex on unregister, so that unregister waits for the ioctls using it
> and later ones fail with -ENODEV. PPS_KC_BIND does the same for a
> removed PPS device since commit 3649f9a6b897 ("pps: don't allow
> PPS_KC_BIND on removed devices"). The sysfs attributes need nothing
> new: they are removed, and their callbacks drained, before info is
> cleared.
>
> Fixes: 86b525bed275 ("drivers pps: add PPS generators support")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Danish Khateeb <danishkhateeb03@gmail.com>
> ---
>
> Notes:
>     Tested in the same setup, with patch 1/4 applied. After an unbind, and
>     after unloading pps_gen-dummy with its file open, both ioctls return
>     -ENODEV with no KASAN report, and closing the file is clean. Normal use
>     of pps_gen-dummy is unchanged.
>
>  drivers/pps/generators/pps_gen.c | 31 ++++++++++++++++++++++++++-----
>  include/linux/pps_gen_kernel.h   |  2 ++
>  2 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/pps/generators/pps_gen.c b/drivers/pps/generators/pps_gen.c
> index 059f4fe6c9b4..d80e28dc31dc 100644
> --- a/drivers/pps/generators/pps_gen.c
> +++ b/drivers/pps/generators/pps_gen.c
> @@ -69,17 +69,28 @@ static long pps_gen_cdev_ioctl(struct file *file,
>  		if (ret)
>  			return -EFAULT;
>  
> -		ret = pps_gen->info->enable(pps_gen, status);
> -		if (ret)
> -			return ret;
> -		pps_gen->enabled = status;
> +		scoped_guard(mutex, &pps_gen->info_lock) {
> +			if (!pps_gen->info)
> +				return -ENODEV;
> +
> +			ret = pps_gen->info->enable(pps_gen, status);
> +			if (ret)
> +				return ret;
> +			pps_gen->enabled = status;
> +		}
>  
>  		break;
>  
>  	case PPS_GEN_USESYSTEMCLOCK:
>  		dev_dbg(&pps_gen->dev, "PPS_GEN_USESYSTEMCLOCK\n");
>  
> -		ret = put_user(pps_gen->info->use_system_clock, uiuarg);
> +		scoped_guard(mutex, &pps_gen->info_lock) {
> +			if (!pps_gen->info)
> +				return -ENODEV;
> +			status = pps_gen->info->use_system_clock;
> +		}
> +
> +		ret = put_user(status, uiuarg);
>  		if (ret)
>  			return -EFAULT;
>  
> @@ -212,6 +223,15 @@ static void pps_gen_unregister_cdev(struct pps_gen_device *pps_gen)
>  {
>  	pr_debug("unregistering pps-gen%d\n", pps_gen->id);
>  	cdev_device_del(&pps_gen->cdev, &pps_gen->dev);
> +
> +	/*
> +	 * An open file keeps pps_gen around, but the driver may free info as
> +	 * soon as we return. The sysfs files are gone now, so wait for the
> +	 * ioctls using info and make later ones fail.
> +	 */
> +	scoped_guard(mutex, &pps_gen->info_lock)
> +		pps_gen->info = NULL;
> +
>  	put_device(&pps_gen->dev);
>  }
>  
> @@ -243,6 +263,7 @@ struct pps_gen_device *pps_gen_register_source(const struct pps_gen_source_info
>  	pps_gen->info = info;
>  	pps_gen->enabled = false;
>  
> +	mutex_init(&pps_gen->info_lock);
>  	init_waitqueue_head(&pps_gen->queue);
>  	spin_lock_init(&pps_gen->lock);
>  
> diff --git a/include/linux/pps_gen_kernel.h b/include/linux/pps_gen_kernel.h
> index f26f6aac000d..c4d22b1af118 100644
> --- a/include/linux/pps_gen_kernel.h
> +++ b/include/linux/pps_gen_kernel.h
> @@ -11,6 +11,7 @@
>  #include <linux/pps_gen.h>
>  #include <linux/cdev.h>
>  #include <linux/device.h>
> +#include <linux/mutex.h>
>  
>  /*
>   * Global defines
> @@ -44,6 +45,7 @@ struct pps_gen_source_info {
>  /* The main struct */
>  struct pps_gen_device {
>  	const struct pps_gen_source_info *info;	/* PSS generator info */
> +	struct mutex info_lock;			/* protects info */
>  	bool enabled;				/* PSS generator status */
>  
>  	unsigned int event;

Acked-by: Rodolfo Giometti <giometti@enneenne.com>