From f0f6ff219b9cb934848c3d2334aa53a034a63554 Mon Sep 17 00:00:00 2001 From: Robert Baldyga Date: Mon, 27 Jun 2022 22:31:48 +0200 Subject: [PATCH 1/4] Set core volume type in metadata on core insert ocf_metadata_flush_superblock() is being called on the cache stop, after deinitialization of the cores (and their volumes), thus accessing core volume in superblock flushing procedure leads to use-after-free bug. Fix this by moving volume type setting to the core insertion code. Signed-off-by: Robert Baldyga --- src/metadata/metadata_superblock.c | 18 ------------------ src/mngt/ocf_mngt_core.c | 3 ++- 2 files changed, 2 insertions(+), 19 deletions(-) diff --git a/src/metadata/metadata_superblock.c b/src/metadata/metadata_superblock.c index f237971..ef0be1f 100644 --- a/src/metadata/metadata_superblock.c +++ b/src/metadata/metadata_superblock.c @@ -440,23 +440,6 @@ void ocf_metadata_load_superblock_recovery(ocf_cache_t cache, ocf_pipeline_next(pipeline); } -static void ocf_metadata_flush_superblock_prepare(ocf_pipeline_t pipeline, - void *priv, ocf_pipeline_arg_t arg) -{ - struct ocf_metadata_context *context = priv; - ocf_cache_t cache = context->cache; - ocf_core_t core; - ocf_core_id_t core_id; - - /* Synchronize core objects types */ - for_each_core_metadata(cache, core, core_id) { - core->conf_meta->type = ocf_ctx_get_volume_type_id( - cache->owner, core->volume.type); - } - - ocf_pipeline_next(pipeline); -} - static void ocf_metadata_flush_superblock_flap(ocf_pipeline_t pipeline, void *priv, ocf_pipeline_arg_t arg) { @@ -544,7 +527,6 @@ struct ocf_pipeline_properties ocf_metadata_flush_sb_pipeline_props = { .priv_size = sizeof(struct ocf_metadata_context), .finish = ocf_metadata_flush_superblock_finish, .steps = { - OCF_PL_STEP(ocf_metadata_flush_superblock_prepare), OCF_PL_STEP_FOREACH(ocf_metadata_calculate_crc, ocf_metadata_flush_sb_args), OCF_PL_STEP_FOREACH(ocf_metadata_flush_segment, diff --git a/src/mngt/ocf_mngt_core.c b/src/mngt/ocf_mngt_core.c index de29774..afdc6e9 100644 --- a/src/mngt/ocf_mngt_core.c +++ b/src/mngt/ocf_mngt_core.c @@ -1,5 +1,5 @@ /* - * Copyright(c) 2012-2021 Intel Corporation + * Copyright(c) 2012-2022 Intel Corporation * SPDX-License-Identifier: BSD-3-Clause */ @@ -426,6 +426,7 @@ static void ocf_mngt_cache_add_core_insert(ocf_pipeline_t pipeline, } core->conf_meta->length = length; + core->conf_meta->type = cfg->volume_type; if (ocf_cache_is_device_attached(cache)) { result = ocf_cleaning_add_core(cache, core_id); From b6587ad622294940c91f64a1fea8260a9b4a75f1 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 27 Jun 2022 14:44:22 +0200 Subject: [PATCH 2/4] zero volume->type in ocf_volume_deinit() After deinitialization of volume there is no need to call back to type ops. Currently we would erroneously call on_deinit() callback multiple times if ocf_volume_deinit() is performed more than once, which we expect to happen and treat as a correct use of API. Signed-off-by: Adam Rutkowski --- src/ocf_volume.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/ocf_volume.c b/src/ocf_volume.c index 4ae8321..5f9fa22 100644 --- a/src/ocf_volume.c +++ b/src/ocf_volume.c @@ -161,6 +161,7 @@ void ocf_volume_deinit(ocf_volume_t volume) env_free(volume->priv); volume->priv = NULL; + volume->type = NULL; if (volume->uuid_copy && volume->uuid.data) { env_vfree(volume->uuid.data); @@ -191,6 +192,7 @@ void ocf_volume_move(ocf_volume_t volume, ocf_volume_t from) from->opened = false; from->priv = NULL; from->uuid.data = NULL; + from->type = NULL; } int ocf_volume_create(ocf_volume_t *volume, ocf_volume_type_t type, From 364e36ec7e181e6cb87a823f7c2ae6f8d9fad3f3 Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 27 Jun 2022 14:52:23 +0200 Subject: [PATCH 3/4] Revert "fix deinitialization of moved composite volume" The proper way to avoid calling on_deinit() callback on an already deinitialized volume is to deinitialize type callbacks, as it is done in the previous commit. This reverts commit a7f70687a97943a4f1ae7301541b5ec1a6307424. Signed-off-by: Adam Rutkowski --- src/ocf_composite_volume.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/ocf_composite_volume.c b/src/ocf_composite_volume.c index a6bf91f..d1530ec 100644 --- a/src/ocf_composite_volume.c +++ b/src/ocf_composite_volume.c @@ -152,11 +152,6 @@ static void ocf_composite_volume_on_deinit(ocf_volume_t cvolume) struct ocf_composite_volume *composite = ocf_volume_get_priv(cvolume); int i; - /* priv can be NULL if this volume had been moved from. In this case - * it's the owner responsibility to deinit member volumes. */ - if (!composite) - return; - for (i = 0; i < composite->members_cnt; i++) ocf_volume_deinit(&composite->member[i].volume); } From 5a71f7c06898582015b1f5fe8c5cc754577d893e Mon Sep 17 00:00:00 2001 From: Adam Rutkowski Date: Mon, 27 Jun 2022 14:49:38 +0200 Subject: [PATCH 4/4] validate uuid->size in ocf_volume_init Optional uuid parameter to ocf_volume_init() points to UUID object initialized by the user. We should verify it is not excesively large as we attempt to allocate a buffer to store a copy of the UUID. Signed-off-by: Adam Rutkowski --- src/ocf_volume.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/ocf_volume.c b/src/ocf_volume.c index 5f9fa22..bf64ec2 100644 --- a/src/ocf_volume.c +++ b/src/ocf_volume.c @@ -93,6 +93,9 @@ int ocf_volume_init(ocf_volume_t volume, ocf_volume_type_t type, if (!volume || !type) return -OCF_ERR_INVAL; + if (uuid && uuid->size > OCF_VOLUME_UUID_MAX_SIZE) + return -OCF_ERR_INVAL; + priv_size = type->properties->volume_priv_size; volume->priv = env_zalloc(priv_size, ENV_MEM_NORMAL); if (!volume->priv)