linux-kernel - [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: <34157f5e8dbaa1063dd76608e1e57244305460e8.1641185192.git.dxu@dxuuu.xyz>
Date:   Sun,  2 Jan 2022 21:01:39 -0800
From:   Daniel Xu <dxu@...uu.xyz>
To:     arnd@...db.de, gregkh@...uxfoundation.org, giometti@...eenne.com,
        linux-kernel@...r.kernel.org
Cc:     Daniel Xu <dxu@...uu.xyz>, thesven73@...il.com, ojeda@...nel.org
Subject: [RFC char-misc-next 1/2] cdev: Add private pointer to struct cdev

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

 void cdev_init(struct cdev *, const struct file_operations *);
-- 
2.34.1