Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
From:   Yu Wang <quic_yyuwang@quicinc.com>
To:     <johannes@sipsolutions.net>, <gregkh@linuxfoundation.org>,
        <rafael@kernel.org>
CC:     <linux-kernel@vger.kernel.org>, <kernel@quicinc.com>
Subject: [PATCH] Devcoredump: fix use-after-free issue when releasing devcd device
Date:   Thu, 26 Oct 2023 22:55:21 -0700
Message-ID: <20231027055521.2679-1-quic_yyuwang@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

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@quicinc.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