From 192e64bcc53130bade44a3b2e405ec8e6e10b2dd Mon Sep 17 00:00:00 2001 From: Jonas Pfefferle Date: Wed, 4 May 2022 12:36:42 +0200 Subject: [PATCH] bdev: spdk_bdev_ext_io_opts missing size check ext_io_opts uses the size member to allow backwards compatibility however currently we only check if it is below or equal the current size of the opts struct and that it is not 0. size is only used when we copy opts because of split or push/pull. This patch introduces size checks to allow safe access to e.g. metadata and memory domain pointers of the user provided opts pointer. The minimum size of the struct passed is now the size of the initial version of spdk_bdev_ext_io_opts. To not introduce additional checks when opts are consumed by a bdev module we now always copy if the size is smaller than the current opts struct size. When introducing new members to opts additional checks might be needed if those are directly accessed through the passed pointer or bdev_io->internal.ext_opts. Change-Id: Ibd181a5840a3d5022018a9f61403df961ffd6e1d Signed-off-by: Jonas Pfefferle Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12550 Community-CI: Broadcom CI Community-CI: Mellanox Build Bot Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Tomasz Zawadzki --- lib/bdev/bdev.c | 19 +++++---- test/unit/lib/bdev/bdev.c/bdev_ut.c | 64 ++++------------------------- 2 files changed, 19 insertions(+), 64 deletions(-) diff --git a/lib/bdev/bdev.c b/lib/bdev/bdev.c index b3e4d200f..2f7c491ac 100644 --- a/lib/bdev/bdev.c +++ b/lib/bdev/bdev.c @@ -1978,10 +1978,6 @@ spdk_bdev_free_io(struct spdk_bdev_io *bdev_io) bdev_io_put_buf(bdev_io); } - if (spdk_unlikely(bdev_io->internal.ext_opts == &bdev_io->internal.ext_opts_copy)) { - memset(&bdev_io->internal.ext_opts_copy, 0, sizeof(bdev_io->internal.ext_opts_copy)); - } - if (ch->per_thread_cache_count < ch->bdev_io_cache_size) { ch->per_thread_cache_count++; STAILQ_INSERT_HEAD(&ch->per_thread_cache, bdev_io, internal.buf_link); @@ -2926,8 +2922,11 @@ _bdev_io_copy_ext_opts(struct spdk_bdev_io *bdev_io, struct spdk_bdev_ext_io_opt { struct spdk_bdev_ext_io_opts *opts_copy = &bdev_io->internal.ext_opts_copy; + /* Zero part we don't copy */ + memset(((char *)opts_copy) + opts->size, 0, sizeof(*opts) - opts->size); memcpy(opts_copy, opts, opts->size); - bdev_io->internal.ext_opts_copy.metadata = bdev_io->u.bdev.md_buf; + opts_copy->size = sizeof(*opts_copy); + opts_copy->metadata = bdev_io->u.bdev.md_buf; /* Save pointer to the copied ext_opts which will be used by bdev modules */ bdev_io->u.bdev.ext_opts = opts_copy; } @@ -2959,7 +2958,7 @@ _bdev_io_submit_ext(struct spdk_bdev_desc *desc, struct spdk_bdev_io *bdev_io, * copy if size is smaller than opts struct to avoid having to check size * on every access to bdev_io->u.bdev.ext_opts */ - if (copy_opts || use_pull_push) { + if (copy_opts || use_pull_push || opts->size < sizeof(*opts)) { _bdev_io_copy_ext_opts(bdev_io, opts); if (use_pull_push) { _bdev_io_ext_use_bounce_buffer(bdev_io); @@ -4307,7 +4306,13 @@ spdk_bdev_readv_blocks_with_md(struct spdk_bdev_desc *desc, struct spdk_io_chann static inline bool _bdev_io_check_opts(struct spdk_bdev_ext_io_opts *opts, struct iovec *iov) { - return opts->size > 0 && + /* + * We check if opts size is at least of size when we first introduced + * spdk_bdev_ext_io_opts (ac6f2bdd8d) since access to those members + * are not checked internal. + */ + return opts->size >= offsetof(struct spdk_bdev_ext_io_opts, metadata) + + sizeof(opts->metadata) && opts->size <= sizeof(*opts) && /* When memory domain is used, the user must provide data buffers */ (!opts->memory_domain || (iov && iov[0].iov_base)); diff --git a/test/unit/lib/bdev/bdev.c/bdev_ut.c b/test/unit/lib/bdev/bdev.c/bdev_ut.c index e2cf897b8..8afbac6ae 100644 --- a/test/unit/lib/bdev/bdev.c/bdev_ut.c +++ b/test/unit/lib/bdev/bdev.c/bdev_ut.c @@ -5094,6 +5094,13 @@ bdev_writev_readv_ext(void) rc = spdk_bdev_writev_blocks_ext(desc, io_ch, &iov, 1, 32, 14, io_done, NULL, &ext_io_opts); CU_ASSERT(rc != 0); + ext_io_opts.size = offsetof(struct spdk_bdev_ext_io_opts, metadata) + + sizeof(ext_io_opts.metadata) - 1; + rc = spdk_bdev_readv_blocks_ext(desc, io_ch, &iov, 1, 32, 14, io_done, NULL, &ext_io_opts); + CU_ASSERT(rc != 0); + rc = spdk_bdev_writev_blocks_ext(desc, io_ch, &iov, 1, 32, 14, io_done, NULL, &ext_io_opts); + CU_ASSERT(rc != 0); + /* Test 3, Check that IO request with ext_opts and metadata is split correctly * Offset 14, length 8, payload 0xF000 * Child - Offset 14, length 2, payload 0xF000 @@ -5152,63 +5159,6 @@ bdev_writev_readv_ext(void) CU_ASSERT(g_io_done == true); CU_ASSERT(g_bdev_ut_channel->outstanding_io_count == 0); - /* ext_opts contains metadata but ext_opts::size doesn't cover metadata pointer. - * Check that IO request with ext_opts and metadata is split correctly - metadata pointer is not copied, so split - * requests do not have metadata - * Offset 14, length 8, payload 0xF000 - * Child - Offset 14, length 2, payload 0xF000 - * Child - Offset 16, length 6, payload 0xF000 + 2 * 512 - */ - iov.iov_base = (void *)0xF000; - iov.iov_len = 4096; - memset(&ext_io_opts, 0, sizeof(ext_io_opts)); - ext_io_opts.metadata = (void *)0xFF000000; - ext_io_opts.size = offsetof(struct spdk_bdev_ext_io_opts, metadata);; - g_io_done = false; - - /* read */ - expected_io = ut_alloc_expected_io(SPDK_BDEV_IO_TYPE_READ, 14, 2, 1); - /* Split request must not contain metadata pointer */ - expected_io->md_buf = NULL; - ut_expected_io_set_iov(expected_io, 0, (void *)0xF000, 2 * 512); - TAILQ_INSERT_TAIL(&g_bdev_ut_channel->expected_io, expected_io, link); - - expected_io = ut_alloc_expected_io(SPDK_BDEV_IO_TYPE_READ, 16, 6, 1); - expected_io->md_buf = NULL; - ut_expected_io_set_iov(expected_io, 0, (void *)(0xF000 + 2 * 512), 6 * 512); - TAILQ_INSERT_TAIL(&g_bdev_ut_channel->expected_io, expected_io, link); - - rc = spdk_bdev_readv_blocks_ext(desc, io_ch, &iov, 1, 14, 8, io_done, NULL, &ext_io_opts); - CU_ASSERT(rc == 0); - CU_ASSERT(g_io_done == false); - - CU_ASSERT(g_bdev_ut_channel->outstanding_io_count == 2); - stub_complete_io(2); - CU_ASSERT(g_io_done == true); - CU_ASSERT(g_bdev_ut_channel->outstanding_io_count == 0); - - /* write */ - g_io_done = false; - expected_io = ut_alloc_expected_io(SPDK_BDEV_IO_TYPE_WRITE, 14, 2, 1); - expected_io->md_buf = NULL; - ut_expected_io_set_iov(expected_io, 0, (void *)0xF000, 2 * 512); - TAILQ_INSERT_TAIL(&g_bdev_ut_channel->expected_io, expected_io, link); - - expected_io = ut_alloc_expected_io(SPDK_BDEV_IO_TYPE_WRITE, 16, 6, 1); - /* Split request must not contain metadata pointer */ - expected_io->md_buf = NULL; - ut_expected_io_set_iov(expected_io, 0, (void *)(0xF000 + 2 * 512), 6 * 512); - TAILQ_INSERT_TAIL(&g_bdev_ut_channel->expected_io, expected_io, link); - - rc = spdk_bdev_writev_blocks_ext(desc, io_ch, &iov, 1, 14, 8, io_done, NULL, &ext_io_opts); - CU_ASSERT(rc == 0); - CU_ASSERT(g_io_done == false); - - CU_ASSERT(g_bdev_ut_channel->outstanding_io_count == 2); - stub_complete_io(2); - CU_ASSERT(g_io_done == true); - CU_ASSERT(g_bdev_ut_channel->outstanding_io_count == 0); - /* Test 4, Verify data pull/push * bdev doens't support memory domains, so buffers from bdev memory pool will be used */ ext_io_opts.memory_domain = (struct spdk_memory_domain *)0xdeadbeef;