ivampiresp/Spdk
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

bdev: spdk_bdev_ext_io_opts missing size check

Browse Source 
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 <pepperjo@japf.ch>
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/12550
Community-CI: Broadcom CI <spdk-ci.pdl@broadcom.com>
Community-CI: Mellanox Build Bot
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
This commit is contained in:
Jonas Pfefferle 2022-05-04 12:36:42 +02:00 committed by Tomasz Zawadzki
parent bf642c08a4
commit 192e64bcc5
2 changed files with 19 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
lib/bdev/bdev.c
View File

@ -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));

64
test/unit/lib/bdev/bdev.c/bdev_ut.c
View File

@ -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;