From 415fa16403bc2503dd1bde17da1113f2fce3e73d Mon Sep 17 00:00:00 2001 From: Alexey Marchuk Date: Tue, 20 Dec 2022 14:11:20 +0100 Subject: [PATCH] util: Add spdk_memset_s bdev_crypto uses memset() to zero secrets passed by the user (cleanup/error path) which is not safe - compiler may detect that the buffer being zeroed is not accessed any more and may "optimize" (drop) zerofying. C11 standard introduces memset_s which guarantess to change the buffer content, but this function is optional, gcc may not support it. As alternative, add not optimal from performance point of view default implementation. Add unit test to math_ut.c to avoid creating new .c file for 1 simple test Signed-off-by: Alexey Marchuk Change-Id: I11c7d15610df02e4a3761a88c85f6f8c54fb4b0a Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/16038 Tested-by: SPDK CI Jenkins Community-CI: Mellanox Build Bot Reviewed-by: Jim Harris Reviewed-by: Tomasz Zawadzki Reviewed-by: Paul Luse Reviewed-by: Shuhei Matsumoto Reviewed-by: Ben Walker --- include/spdk/util.h | 35 +++++++++++++++++++++++++++++ lib/util/spdk_util.map | 1 + test/unit/lib/util/math.c/math_ut.c | 27 +++++++++++++++++++--- 3 files changed, 60 insertions(+), 3 deletions(-) diff --git a/include/spdk/util.h b/include/spdk/util.h index 9de149ad2..12041c537 100644 --- a/include/spdk/util.h +++ b/include/spdk/util.h @@ -10,6 +10,9 @@ #ifndef SPDK_UTIL_H #define SPDK_UTIL_H +/* memset_s is only available if __STDC_WANT_LIB_EXT1__ is set to 1 before including \ */ +#define __STDC_WANT_LIB_EXT1__ 1 + #include "spdk/stdinc.h" #ifdef __cplusplus @@ -230,6 +233,38 @@ spdk_sn32_gt(uint32_t s1, uint32_t s2) (s1 > s2 && s1 - s2 < SPDK_SN32_CMPMAX)); } +/** + * Copies the value (unsigned char)ch into each of the first \b count characters of the object pointed to by \b data + * \b data_size is used to check that filling \b count bytes won't lead to buffer overflow + * + * \param data Buffer to fill + * \param data_size Size of the buffer + * \param ch Fill byte + * \param count Number of bytes to fill + */ +static inline void +spdk_memset_s(void *data, size_t data_size, int ch, size_t count) +{ +#ifdef __STDC_LIB_EXT1__ + /* memset_s was introduced as an optional feature in C11 */ + memset_s(data, data_size, ch, count); +#else + size_t i; + volatile unsigned char *buf = (volatile unsigned char *)data; + + if (!buf) { + return; + } + if (count > data_size) { + count = data_size; + } + + for (i = 0; i < count; i++) { + buf[i] = (unsigned char)ch; + } +#endif +} + #ifdef __cplusplus } #endif diff --git a/lib/util/spdk_util.map b/lib/util/spdk_util.map index f5af89460..65d1b354e 100644 --- a/lib/util/spdk_util.map +++ b/lib/util/spdk_util.map @@ -136,6 +136,7 @@ spdk_ioviter_next; spdk_copy_iovs_to_buf; spdk_copy_buf_to_iovs; + spdk_memset_s; # resolvers for functions in util.h spdk_u32log2.resolver; diff --git a/test/unit/lib/util/math.c/math_ut.c b/test/unit/lib/util/math.c/math_ut.c index 8b22db277..e7596a13a 100644 --- a/test/unit/lib/util/math.c/math_ut.c +++ b/test/unit/lib/util/math.c/math_ut.c @@ -1,5 +1,6 @@ /* SPDX-License-Identifier: BSD-3-Clause * Copyright (C) 2020 Intel Corporation. + * Copyright (c) 2022, NVIDIA CORPORATION & AFFILIATES. * All rights reserved. */ @@ -29,18 +30,38 @@ test_serial_number_arithmetic(void) CU_ASSERT(spdk_sn32_lt(100, UINT32_MAX - 100) == false); } +static void +test_memset_s(void) +{ + char secret[] = "0123456789abcdef"; + + /* Zero length, nothing should be changed */ + spdk_memset_s(secret, sizeof(secret), 'b', 0); + CU_ASSERT_EQUAL(memcmp(secret, "0123456789abcdef", sizeof(secret)), 0); + + /* Fill digits */ + spdk_memset_s(secret, sizeof(secret), 'x', 10); + CU_ASSERT_EQUAL(memcmp(secret, "xxxxxxxxxxabcdef", sizeof(secret)), 0); + + /* Fill the whole string except of the NULL char */ + spdk_memset_s(secret, sizeof(secret), 'y', sizeof(secret) - 1); + CU_ASSERT_EQUAL(memcmp(secret, "yyyyyyyyyyyyyyyy", sizeof(secret) - 1), 0); +} + int main(int argc, char **argv) { - CU_pSuite suite = NULL; + CU_pSuite suite_math = NULL, suite_erase = NULL; unsigned int num_failures; CU_set_error_action(CUEA_ABORT); CU_initialize_registry(); - suite = CU_add_suite("math", NULL, NULL); + suite_math = CU_add_suite("math", NULL, NULL); + CU_ADD_TEST(suite_math, test_serial_number_arithmetic); - CU_ADD_TEST(suite, test_serial_number_arithmetic); + suite_erase = CU_add_suite("erase", NULL, NULL); + CU_ADD_TEST(suite_erase, test_memset_s); CU_basic_set_mode(CU_BRM_VERBOSE);