linux-kernel - [PATCH] Devcoredump: fix use-after-free issue when releasing devcd device

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-next>] [day] [month] [year] [list]

Message-ID: <20231027055521.2679-1-quic_yyuwang@quicinc.com>
Date:   Thu, 26 Oct 2023 22:55:21 -0700
From:   Yu Wang <quic_yyuwang@...cinc.com>
To:     <johannes@...solutions.net>, <gregkh@...uxfoundation.org>,
        <rafael@...nel.org>
CC:     <linux-kernel@...r.kernel.org>, <kernel@...cinc.com>
Subject: [PATCH] Devcoredump: fix use-after-free issue when releasing devcd device

With sample code as below, it may hit use-after-free issue when
releasing devcd device.

    struct my_coredump_state {
        struct completion dump_done;
        ...
    };

    static void my_coredump_free(void *data)
    {
        struct my_coredump_state *dump_state = data;
        ...
        complete(&dump_state->dump_done);
    }

    static void my_dev_release(struct device *dev)
    {
        kfree(dev);
    }

    static void my_coredump()
    {
        struct my_coredump_state dump_state;
        struct device *new_device =
            kzalloc(sizeof(*new_device), GFP_KERNEL);

        ...
        new_device->release = my_dev_release;
        device_initialize(new_device);
        ...
        device_add(new_device);
        ...
        init_completion(&dump_state.dump_done);
        dev_coredumpm(new_device, NULL, &dump_state, datalen, GFP_KERNEL,
                      my_coredump_read, my_coredump_free);
        wait_for_completion(&dump_state.dump_done);
        device_del(new_device);
        put_device(new_device);
    }

In devcoredump framework, devcd_dev_release() will be called when
releasing the devcd device, it will call the free() callback first
and try to delete the symlink in sysfs directory of the failing device.
Eventhough it has checked 'devcd->failing_dev->kobj.sd' before that,
there is no mechanism to ensure it's still available when accessing
it in kernfs_find_ns(), refer to the diagram as below:

    Thread A was waiting for 'dump_state.dump_done' at #A-1-2 after
    calling dev_coredumpm().
    When thread B calling devcd->free() at #B-2-1, it wakes up
    thread A from point #A-1-2, which will call device_del() to
    delete the device.
    If #B-2-2 comes before #A-3-1, but #B-4 comes after #A-4, it
    will hit use-after-free issue when trying to access
    'devcd->failing_dev->kobj.sd'.

    #A-1-1: dev_coredumpm()
      #A-1-2: wait_for_completion(&dump_state.dump_done)
      #A-1-3: device_del()
        #A-2: kobject_del()
          #A-3-1: sysfs_remove_dir() --> set kobj->sd=NULL
          #A-3-2: kernfs_put()
            #A-4: kmem_cache_free() --> free kobj->sd

    #B-1: devcd_dev_release()
      #B-2-1: devcd->free(devcd->data)
      #B-2-2: check devcd->failing_dev->kobj.sd
      #B-2-3: sysfs_delete_link()
        #B-3: kernfs_remove_by_name_ns()
          #B-4: kernfs_find_ns() --> access devcd->failing_dev->kobj.sd

To fix this issue, put operations on devcd->failing_dev before
calling the free() callback in devcd_dev_release().

Signed-off-by: Yu Wang <quic_yyuwang@...cinc.com>
---
 drivers/base/devcoredump.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/base/devcoredump.c b/drivers/base/devcoredump.c
index 91536ee05f14..35c704ddfeae 100644
--- a/drivers/base/devcoredump.c
+++ b/drivers/base/devcoredump.c
@@ -83,9 +83,6 @@ static void devcd_dev_release(struct device *dev)
 {
 	struct devcd_entry *devcd = dev_to_devcd(dev);
 
-	devcd->free(devcd->data);
-	module_put(devcd->owner);
-
 	/*
 	 * this seems racy, but I don't see a notifier or such on
 	 * a struct device to know when it goes away?
@@ -95,6 +92,8 @@ static void devcd_dev_release(struct device *dev)
 				  "devcoredump");
 
 	put_device(devcd->failing_dev);
+	devcd->free(devcd->data);
+	module_put(devcd->owner);
 	kfree(devcd);
 }
 
-- 
2.17.1