From 7f737cdc93826a36087417004b99c34e7296e9f8 Mon Sep 17 00:00:00 2001 From: Peter Colberg Date: Thu, 17 Nov 2022 18:04:25 -0500 Subject: [PATCH 1/2] fake_bsp: fix stringop-overread warnings with string literals GCC 11 correctly points out that there is no point in limiting strnlen() to a size that is greater than the length of the string literal argument. test/fake_bsp/fakegoodbsp.cpp:47:26: warning: 'size_t strnlen(const char*, size_t)' specified bound 1204 exceeds source size 16 [-Wstringop-overread] 47 | size_t Xlen = strnlen(X, MAX_NAME_SIZE) + 1; \ | ~~~~~~~^~~~~~~~~~~~~~~~~~ strnlen() is commonly used with an input buffer that is possibly not null-terminated, i.e., contains a truncated string. strnlen() is also useful to set an upper bound on the length to consume, e.g., to avoid a denial of service. Neither of these use cases apply here. MAX_NAME_SIZE was previously introduced in acl.h to address Klocwork issues relating to missing null termination of strings. The chosen 1204 is a typo of 1024, but even then, it should be 1023 such that a buffer with the trailing null byte occupies 1023 + 1 = 1024 bytes. Signed-off-by: Peter Colberg --- test/fake_bsp/fakegoodbsp.cpp | 4 ++-- test/fake_bsp/missingfuncbsp.cpp | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/test/fake_bsp/fakegoodbsp.cpp b/test/fake_bsp/fakegoodbsp.cpp index d3cbff2a..79063f8c 100644 --- a/test/fake_bsp/fakegoodbsp.cpp +++ b/test/fake_bsp/fakegoodbsp.cpp @@ -44,7 +44,7 @@ typedef enum { } #define RESULT_STR(X) \ do { \ - size_t Xlen = strnlen(X, MAX_NAME_SIZE) + 1; \ + size_t Xlen = strlen(X) + 1; \ memcpy((void *)param_value, X, \ (param_value_size <= Xlen) ? param_value_size : Xlen); \ if (param_size_ret) \ @@ -283,7 +283,7 @@ AOCL_MMD_CALL int aocl_mmd_read(int handle, aocl_mmd_op_t op, size_t len, (unsigned int)offset); return -1; case OFFSET_CONFIGURATION_ROM: - if (strnlen(config_str, MAX_NAME_SIZE) <= len) { + if (strlen(config_str) <= len) { memcpy(dst, (void *)config_str, len); return 0; } else { diff --git a/test/fake_bsp/missingfuncbsp.cpp b/test/fake_bsp/missingfuncbsp.cpp index 4b2463cc..0879f2e9 100644 --- a/test/fake_bsp/missingfuncbsp.cpp +++ b/test/fake_bsp/missingfuncbsp.cpp @@ -44,7 +44,7 @@ typedef enum { } #define RESULT_STR(X) \ do { \ - size_t Xlen = strnlen(X, MAX_NAME_SIZE) + 1; \ + size_t Xlen = strlen(X) + 1; \ memcpy((void *)param_value, X, \ (param_value_size <= Xlen) ? param_value_size : Xlen); \ if (param_size_ret) \ @@ -268,7 +268,7 @@ AOCL_MMD_CALL int aocl_mmd_read(int handle, aocl_mmd_op_t op, size_t len, (unsigned int)offset); return -1; case OFFSET_CONFIGURATION_ROM: - if (strnlen(config_str, MAX_NAME_SIZE) <= len) { + if (strlen(config_str) <= len) { memcpy(dst, (void *)config_str, len); return 0; } else { From caa98ebd373d48e5ec063d604347248d9df82ba6 Mon Sep 17 00:00:00 2001 From: Peter Colberg Date: Thu, 17 Nov 2022 18:40:39 -0500 Subject: [PATCH 2/2] fake_bsp: fix out-of-bounds read of OFFSET_CONFIGURATION_ROM The check failed to account for the trailing null byte, and memcpy() read beyond the end of the copied string literal. Signed-off-by: Peter Colberg --- test/fake_bsp/fakegoodbsp.cpp | 12 ++++++------ test/fake_bsp/missingfuncbsp.cpp | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/test/fake_bsp/fakegoodbsp.cpp b/test/fake_bsp/fakegoodbsp.cpp index 79063f8c..ef2d75b3 100644 --- a/test/fake_bsp/fakegoodbsp.cpp +++ b/test/fake_bsp/fakegoodbsp.cpp @@ -282,14 +282,14 @@ AOCL_MMD_CALL int aocl_mmd_read(int handle, aocl_mmd_op_t op, size_t len, fprintf(stderr, "Error: Not handling read of 0x%x in unit test\n", (unsigned int)offset); return -1; - case OFFSET_CONFIGURATION_ROM: - if (strlen(config_str) <= len) { - memcpy(dst, (void *)config_str, len); - return 0; - } else { + case OFFSET_CONFIGURATION_ROM: { + const size_t config_len = strlen(config_str); + if (!(config_len < len)) { return -1; } - break; + memcpy(dst, config_str, config_len + 1); + return 0; + } case OFFSET_COUNTER: if (len == sizeof(unsigned int)) { memcpy(dst, &offset_counter, len); diff --git a/test/fake_bsp/missingfuncbsp.cpp b/test/fake_bsp/missingfuncbsp.cpp index 0879f2e9..4c831a12 100644 --- a/test/fake_bsp/missingfuncbsp.cpp +++ b/test/fake_bsp/missingfuncbsp.cpp @@ -267,14 +267,14 @@ AOCL_MMD_CALL int aocl_mmd_read(int handle, aocl_mmd_op_t op, size_t len, fprintf(stderr, "Error: Not handling read of 0x%x in unit test\n", (unsigned int)offset); return -1; - case OFFSET_CONFIGURATION_ROM: - if (strlen(config_str) <= len) { - memcpy(dst, (void *)config_str, len); - return 0; - } else { + case OFFSET_CONFIGURATION_ROM: { + const size_t config_len = strlen(config_str); + if (!(config_len < len)) { return -1; } - break; + memcpy(dst, config_str, config_len + 1); + return 0; + } case OFFSET_COUNTER: fprintf(stderr, "Error: Not handling read of 0x%x in unit test\n", (unsigned int)offset);