From d0038b70df91e9b90aeca4355090761f09f6af0e Mon Sep 17 00:00:00 2001 From: Nathan Claudel Date: Mon, 11 Jul 2022 18:03:50 +0200 Subject: [PATCH] bdev: fix use-after-free in bdev registration When a bdev is registered, it is examined by the bdev modules before the bdev register even is notified. Examination may be asychronous, e.g. when the bdev module has to perform I/O on the new bdev. This causes a race condition where the bdev might be destroyed while examination is not finished. Then, once all modules have signaled that examination is done, `bdev_register_finished` makes an invalid access to the freed bdev pointer. To fix this, defer the unregistration until the examine is completed by opening a descriptor on the bdev. Change-Id: I79a2faa96c1c893fc1cee645fbe31f689b03ea4a Signed-off-by: Nathan Claudel Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/13630 Community-CI: Mellanox Build Bot Reviewed-by: Shuhei Matsumoto Reviewed-by: Jim Harris Reviewed-by: Konrad Sztyber Tested-by: SPDK CI Jenkins --- lib/bdev/bdev.c | 71 +++++++++++++++++++---------- test/unit/lib/bdev/bdev.c/bdev_ut.c | 3 ++ 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index 3d3296f43..11434ede7 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -6163,29 +6163,6 @@ bdev_destroy_cb(void *io_device) } } -static void -bdev_register_finished(void *arg) -{ - struct spdk_bdev *bdev = arg; - - spdk_notify_send("bdev_register", spdk_bdev_get_name(bdev)); -} - -int -spdk_bdev_register(struct spdk_bdev *bdev) -{ - int rc = bdev_register(bdev); - - if (rc == 0) { - /* Examine configuration before initializing I/O */ - bdev_examine(bdev); - - spdk_bdev_wait_for_examine(bdev_register_finished, bdev); - } - - return rc; -} - void spdk_bdev_destruct_done(struct spdk_bdev *bdev, int bdeverrno) { @@ -6584,6 +6561,54 @@ spdk_bdev_close(struct spdk_bdev_desc *desc) pthread_mutex_unlock(&g_bdev_mgr.mutex); } +static void +bdev_register_finished(void *arg) +{ + struct spdk_bdev_desc *desc = arg; + struct spdk_bdev *bdev = spdk_bdev_desc_get_bdev(desc); + + spdk_notify_send("bdev_register", spdk_bdev_get_name(bdev)); + + bdev_close(bdev, desc); +} + +int +spdk_bdev_register(struct spdk_bdev *bdev) +{ + struct spdk_bdev_desc *desc; + int rc; + + rc = bdev_register(bdev); + if (rc != 0) { + return rc; + } + + /* A descriptor is opened to prevent bdev deletion during examination */ + rc = bdev_desc_alloc(bdev, _tmp_bdev_event_cb, NULL, &desc); + if (rc != 0) { + spdk_bdev_unregister(bdev, NULL, NULL); + return rc; + } + + rc = bdev_open(bdev, false, desc); + if (rc != 0) { + bdev_desc_free(desc); + spdk_bdev_unregister(bdev, NULL, NULL); + return rc; + } + + /* Examine configuration before initializing I/O */ + bdev_examine(bdev); + + rc = spdk_bdev_wait_for_examine(bdev_register_finished, desc); + if (rc != 0) { + bdev_close(bdev, desc); + spdk_bdev_unregister(bdev, NULL, NULL); + } + + return rc; +} + int spdk_bdev_module_claim_bdev(struct spdk_bdev *bdev, struct spdk_bdev_desc *desc, struct spdk_bdev_module *module) diff --git a/test/unit/lib/bdev/bdev.c/bdev_ut.c b/test/unit/lib/bdev/bdev.c/bdev_ut.c index a0ba26ead..d9ea183e8 100644 --- a/test/unit/lib/bdev/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/bdev.c/bdev_ut.c @@ -451,6 +451,7 @@ allocate_bdev(char *name) bdev->blocklen = 512; rc = spdk_bdev_register(bdev); + poll_threads(); CU_ASSERT(rc == 0); return bdev; @@ -470,6 +471,7 @@ allocate_vbdev(char *name) bdev->module = &vbdev_ut_if; rc = spdk_bdev_register(bdev); + poll_threads(); CU_ASSERT(rc == 0); return bdev; @@ -807,6 +809,7 @@ num_blocks_test(void) bdev.fn_table = &fn_table; bdev.module = &bdev_ut_if; spdk_bdev_register(&bdev); + poll_threads(); spdk_bdev_notify_blockcnt_change(&bdev, 50); /* Growing block number */