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

[Greg KH]

On Sun, Jan 02, 2022 at 09:01:40PM -0800, Daniel Xu wrote:
> 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@xxxxxxxxx>
> ---
>  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;

Why is this pointer now valid while the original structure that the cdev
lived in, not valid?  I do not think this really solves your problem,
only papers over the delay in removing the kobject that the config
option you enabled is trying to tell you is a problem.

>  	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;

So now who owns the lifecycle for the ppc_device structure?  You just
took it away from the cdev kobject, and replaced it with what?

thanks,

greg k-h