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

[Daniel Xu]

On Mon, Jan 03, 2022 at 03:06:11PM +0100, Greg KH wrote:
> 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.

I'm confused here as well. The original structure that the cdev lived in
is still valid here. I think this is the only way back to the containing
structure if we choose to embed `struct cdev *` rather than `struct cdev`.

Unless you're suggesting the cdev is opened after the containing struct
is already freed. In which case neither the original method (embeddeding
`struct cdev`) nor the private pointer method would save us.

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

Unless I'm misunderstanding, the lifecycle owner has not changed in this
patch. AFAICT (and KASAN seems to agree with me) this is all still
valid.

FWIW other drivers store `struct cdev *` too. See fs/fuse/cuse.c's
cuse_channel_open() and cuse_channel_release().

Sorry about any dumb questions -- still new to driver stuff.

[...]

Thanks,
Daniel