Re: [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev

[Daniel Xu <dxu@dxuuu.xyz>]

Hi Greg,

Thanks for taking the time to respond.

On Mon, Jan 03, 2022 at 03:04:14PM +0100, Greg KH wrote:
> On Sun, Jan 02, 2022 at 09:01:39PM -0800, Daniel Xu wrote:
> > struct cdev is a kobject managed struct, meaning kobject is ultimately
> > responsible for deciding when the object is freed. Because kobject uses
> > reference counts, it also means a cdev object isn't guaranteed to be
> > cleaned up with a call to cdev_del() -- the cleanup may occur later.
> > 
> > Unfortunately, this can result in subtle use-after-free bugs when struct
> > cdev is embedded in another struct, and the larger struct is freed
> > immediately after cdev_del(). For example:
> > 
> >     struct contains_cdev {
> >             struct cdev cdev;
> >     }
> > 
> >     void init(struct contains_cdev *cc) {
> >             cdev_init(&cc->cdev);
> >     }
> > 
> >     void cleanup(struct contains_cdev *cc) {
> >             cdev_del(&cc->cdev);
> >             kfree(cc);
> >     }
> > 
> > This kind of code can reliably trigger a KASAN splat with
> > CONFIG_KASAN=y and CONFIG_DEBUG_KOBJECT_RELEASE=y.
> > 
> > A fairly palatable workaround is replacing cdev_init() with cdev_alloc()
> > and storing a pointer instead. For example, this is totally fine:
> > 
> >     struct contains_cdev_ptr {
> >             struct cdev *cdev;
> >     }
> > 
> >     int init(struct contains_cdev_ptr *cc) {
> >             cc->cdev = cdev_alloc();
> >             if (!cc->cdev) {
> >                     return -ENOMEM;
> >             }
> > 
> >             return 0;
> >     }
> > 
> >     void cleanup(struct contains_cdev_ptr *cc) {
> >             cdev_del(cc->cdev);
> >             kfree(cc);
> >     }
> > 
> > The only downside from this workaround (other than the extra allocation)
> > is that container_of() upcasts no longer work. This is quite unfortunate
> > for any code that implements struct file_operations and wants to store
> > extra data with a struct cdev. With cdev_alloc() pointer, it's no longer
> > possible to do something like:
> > 
> >     struct contains_cdev *cc = container_of(inode->i_cdev,
> >                                             struct contains_cdev,
> >                                             cdev);
> > 
> > Thus, I propose to add a void *private field to struct cdev so that
> > callers can store a pointer to the containing struct instead of using
> > container_of().
> > 
> > Signed-off-by: Daniel Xu <dxu@dxuuu.xyz>
> > ---
> >  include/linux/cdev.h | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/include/linux/cdev.h b/include/linux/cdev.h
> > index 0e8cd6293deb..0e674e900512 100644
> > --- a/include/linux/cdev.h
> > +++ b/include/linux/cdev.h
> > @@ -18,6 +18,7 @@ struct cdev {
> >  	struct list_head list;
> >  	dev_t dev;
> >  	unsigned int count;
> > +	void *private;
> 
> I understand your request here, but this is not how to use a cdev.  It
> should be embedded in a larger structure, and then you can always "cast
> out" to get the real structure here.

Hmm, I see. Is there a recommended method to avoid the use-after-free
issue then? When `struct contains_cdev` is allocated on the heap we must
free it at some point. IOW how do we ensure `struct contains_cdev` is
only freed after the `struct cdev` is freed?

> If you can't do that, then this
> private pointer doesn't make much sense at all as it too would be
> invalid.

I could be misunderstanding something here, but I don't see why the
impossiblity of doing a container_of() implies the private pointer is
also invalid. Could you please elaborate?

Just to be clear, the goal behind this private pointer isn't to access
`struct contains_cdev` via a pointer to cdev after `struct contains_cdev`
is already freed -- it's to avoid a (IMO) previously unavoidable
use-after-free.

I see this private pointer as the same as in `struct file`'s
private_data pointer. There is no contract -- it's all up to the caller.

> Ideally the kobject in the cdev structure would not be used by things
> "outside" of the cdev core, but that ship seems long gone.  So just rely
> on the structure that this kobject protects, and you should be fine.

I'm also confused on this point. `struct cdev` has a kobject inside
which manages the lifetime of cdev instances. But that doesn't protect
any struct that embeds a `struct cdev` though, right?

[...]

Thanks,
Daniel