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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YdMCXierm0K6cVA/@kroah.com>
Date:   Mon, 3 Jan 2022 15:04:14 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Daniel Xu <dxu@...uu.xyz>
Cc:     arnd@...db.de, giometti@...eenne.com, linux-kernel@...r.kernel.org,
        thesven73@...il.com, ojeda@...nel.org
Subject: Re: [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev

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@...uu.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.  If you can't do that, then this
private pointer doesn't make much sense at all as it too would be
invalid.

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.

thanks,

greg k-h