adafruit-mirror/zephyr
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 9

lib: os: cbprintf: Mechanism for detecting %p in static package

Browse source 
Static packaging is using only argument types to build a package. There
is one case where analysing argument type only is not enough to
determine package content. That is %p with (unsigned) char pointer vs
%s. In case of %s a string might need to be appended to the package
and in case of %p it must be avoided. Format string analysis is required
to distinguish those two cases.
In order to speed up the runtime inspection, additional information is
added to a static package. That is index of the string argument (where
first argument has index 0). This information allows quick format string
inspection where nth format specifier is found and checked if it is a
pointer format specifier.
Inspection algorithm is added to cbprintf_package_convert() and if %p
is found then data for that argument is discarded. Additionally, log
warning is printed with suggestion to cast pointer argument to void *
to avoid confusion. It is desired to get rid of this ambiguity because
there are going to be logging configurations where strings are stripped
from a binary and runtime inspection cannot be performed.

Signed-off-by: Krzysztof Chruscinski <krzysztof.chruscinski@nordicsemi.no>
This commit is contained in:
Krzysztof Chruscinski 2022-08-26 14:14:23 +02:00 committed by Fabio Baltieri
parent bd51d0ed20
commit a7224830ce
5 changed files with 151 additions and 43 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

19
include/zephyr/sys/cbprintf.h
View file

@ -214,6 +214,25 @@ BUILD_ASSERT(Z_IS_POW2(CBPRINTF_PACKAGE_ALIGNMENT));
#define CBPRINTF_PACKAGE_CONVERT_KEEP_RO_STR BIT(2)
#define CBPRINTF_PACKAGE_COPY_KEEP_RO_STR CBPRINTF_PACKAGE_CONVERT_KEEP_RO_STR __DEPRECATED_MACRO
/** @brief Check format string if %p argument was treated as %s in the package.
*
* Static packaging is done based only on types of arguments used for a format
* string. Without looking into format specifiers present in the string. Because
* of that if (unsigned) char pointer is used for %p it will be considered as
* a string location and during conversion an attempt to append a string to a
* package may be performed. This can lead to misbehavior, in the best case
* package will be bigger and in the worst case memory fault or security violation
* may occur.
*
* When this flag is set, format string will be checked to detect cases when
* string candidate is a pointer used for %p and string appending from unexpected
* location is avoided. Additionally, an log warning is generated to encourage
* user to cast such argument to void *. It is recommended because there are
* configurations where string is not accessible and inspection cannot be done.
* In those cases there are no means to detect such cases.
*/
#define CBPRINTF_PACKAGE_CONVERT_PTR_CHECK BIT(3)
/**@} */
/**@defgroup Z_CBVPRINTF_PROCESS_FLAGS cbvprintf processing flags.

8
include/zephyr/sys/cbprintf_internal.h
View file

@ -312,6 +312,7 @@ do { \
if (_cros_en) { \
if (Z_CBPRINTF_IS_X_PCHAR(arg_idx, _arg, _flags)) { \
if (_rws_pos_en) { \
_rws_buffer[_rws_pos_idx++] = arg_idx - 1; \
_rws_buffer[_rws_pos_idx++] = _loc; \
} \
} else { \
@ -320,6 +321,7 @@ do { \
} \
} \
} else if (_rws_pos_en) { \
_rws_buffer[_rws_pos_idx++] = arg_idx - 1; \
_rws_buffer[_rws_pos_idx++] = _idx / sizeof(int); \
} \
} \
@ -424,7 +426,7 @@ do { \
uint8_t *_ros_pos_buf; \
Z_CBPRINTF_ON_STACK_ALLOC(_ros_pos_buf, _ros_cnt); \
uint8_t *_rws_buffer; \
Z_CBPRINTF_ON_STACK_ALLOC(_rws_buffer, _rws_cnt); \
Z_CBPRINTF_ON_STACK_ALLOC(_rws_buffer, 2 * _rws_cnt); \
size_t _pmax = (buf != NULL) ? _inlen : INT32_MAX; \
int _pkg_len = 0; \
int _total_len = 0; \
@ -448,14 +450,14 @@ do { \
_total_len = _pkg_len; \
/* Append string indexes to the package. */ \
_total_len += _ros_cnt; \
_total_len += _rws_cnt; \
_total_len += 2 * _rws_cnt; \
if (_pbuf != NULL) { \
/* Append string locations. */ \
uint8_t *_pbuf_loc = &_pbuf[_pkg_len]; \
for (size_t i = 0; i < _ros_cnt; i++) { \
*_pbuf_loc++ = _ros_pos_buf[i]; \
} \
for (size_t i = 0; i < _rws_cnt; i++) { \
for (size_t i = 0; i < (2 * _rws_cnt); i++) { \
*_pbuf_loc++ = _rws_buffer[i]; \
} \
} \

4
lib/os/Kconfig.cbprintf
View file

@ -118,6 +118,10 @@ config CBPRINTF_LIBC_SUBSTS
When used with CBPRINTF_NANO this increases the implementation code
size by a small amount.
module = CBPRINTF_PACKAGE
module-str = cbprintf_package
source "subsys/logging/Kconfig.template.log_config"
config CBPRINTF_PACKAGE_LONGDOUBLE
bool "Support packaging of long doubles"
help

122
lib/os/cbprintf_packaged.c
View file

@ -14,6 +14,8 @@
#include <sys/types.h>
#include <zephyr/sys/util.h>
#include <zephyr/sys/__assert.h>
#include <zephyr/logging/log.h>
LOG_MODULE_REGISTER(cbprintf_package, CONFIG_CBPRINTF_PACKAGE_LOG_LEVEL);
#if defined(CONFIG_CBPRINTF_PACKAGE_SUPPORT_TAGGED_ARGUMENTS) && \
!Z_C_GENERIC
@ -247,9 +249,11 @@ int cbvprintf_package(void *packaged, size_t len, uint32_t flags,
unsigned int size; /* current argument's size */
unsigned int align; /* current argument's required alignment */
uint8_t str_ptr_pos[16]; /* string pointer positions */
uint8_t str_ptr_arg[16]; /* string pointer argument index */
unsigned int s_idx = 0; /* index into str_ptr_pos[] */
unsigned int s_rw_cnt = 0; /* number of rw strings */
unsigned int s_ro_cnt = 0; /* number of ro strings */
int arg_idx = -1; /* Argument index. Preincremented thus starting from -1.*/
unsigned int i;
const char *s;
bool parsing = false;
@ -468,6 +472,7 @@ int cbvprintf_package(void *packaged, size_t len, uint32_t flags,
if (!parsing) {
if (*fmt == '%') {
parsing = true;
arg_idx++;
align = VA_STACK_ALIGN(int);
size = sizeof(int);
}
@ -476,6 +481,7 @@ int cbvprintf_package(void *packaged, size_t len, uint32_t flags,
switch (*fmt) {
case '%':
parsing = false;
arg_idx--;
continue;
case '#':
@ -641,6 +647,7 @@ process_string:
* We will append non-ro strings later.
*/
str_ptr_pos[s_idx] = s_ptr_idx;
str_ptr_arg[s_idx] = arg_idx;
if (is_ro) {
/* flag read-only string. */
str_ptr_pos[s_idx] |= STR_POS_RO_FLAG;
@ -648,12 +655,18 @@ process_string:
} else {
s_rw_cnt++;
}
} else if (is_ro || rws_pos_en) {
} else if (is_ro) {
/*
* Add only pointer position prefix
* when counting strings.
*/
len += 1;
} else if (rws_pos_en) {
/*
* Add only pointer position prefix and
* argument index when counting strings.
*/
len += 2;
} else {
/*
* Add the string length, the final '\0'
@ -766,6 +779,7 @@ process_string:
if (rws_pos_en) {
size = 0;
*buf++ = str_ptr_arg[i];
} else {
/* retrieve the string pointer */
s = *(char **)(buf0 + str_ptr_pos[i] * sizeof(int));
@ -830,7 +844,7 @@ int cbpprintf_external(cbprintf_cb out,
rws_nbr = hdr->hdr.desc.rw_str_cnt;
/* Locate the string table */
s = (char *)(buf + args_size + ros_nbr + rws_nbr);
s = (char *)(buf + args_size + ros_nbr + 2 * rws_nbr);
/*
* Patch in string pointers.
@ -852,6 +866,45 @@ int cbpprintf_external(cbprintf_cb out,
return cbprintf_via_va_list(out, formatter, ctx, hdr->fmt, buf);
}
/* Function checks if character might be format specifier. Check is relaxed since
* compiler ensures that correct format specifier is used so it is enough to check
* that character is not one of potential modifier (e.g. number, dot, etc.).
*/
static bool is_fmt_spec(char c)
{
return (c >= 64) && (c <= 122);
}
/* Function checks if nth argument is a pointer (%p). Returns true is yes. Returns
* false if not or if string does not have nth argument.
*/
bool is_ptr(const char *fmt, int n)
{
char c;
bool mod = false;
int cnt = 0;
while ((c = *fmt++) != '\0') {
if (mod) {
if (cnt == n) {
if (c == 'p') {
return true;
} else if (is_fmt_spec(c)) {
return false;
}
} else if (is_fmt_spec(c)) {
cnt++;
mod = false;
}
}
if (c == '%') {
mod = !mod;
}
}
return false;
}
int cbprintf_package_convert(void *in_packaged,
size_t in_len,
cbprintf_convert_cb cb,
@ -865,6 +918,7 @@ int cbprintf_package_convert(void *in_packaged,
uint8_t *buf = in_packaged;
uint32_t *buf32 = in_packaged;
unsigned int args_size, ros_nbr, rws_nbr;
bool fmt_present = flags & CBPRINTF_PACKAGE_CONVERT_PTR_CHECK ? true : false;
bool rw_cpy;
bool ro_cpy;
struct cbprintf_package_desc *in_desc = in_packaged;
@ -908,6 +962,7 @@ int cbprintf_package_convert(void *in_packaged,
/* Pointer to array with string locations. Array starts with read-only
* string locations.
*/
const char *fmt = *(const char **)(buf + sizeof(void *));
uint8_t *str_pos = &buf[args_size];
size_t strl_cnt = 0;
@ -938,28 +993,43 @@ int cbprintf_package_convert(void *in_packaged,
/* Handle RW strings. */
for (int i = 0; i < rws_nbr; i++) {
const char *str = *(const char **)&buf32[*str_pos];
uint8_t arg_idx = *str_pos++;
uint8_t arg_pos = *str_pos++;
const char *str = *(const char **)&buf32[arg_pos];
bool is_ro = ptr_in_rodata(str);
int len;
if ((is_ro && flags & CBPRINTF_PACKAGE_CONVERT_RO_STR) ||
(!is_ro && flags & CBPRINTF_PACKAGE_CONVERT_RW_STR)) {
int len = append_string(cb, NULL, str, 0);
if (fmt_present && is_ptr(fmt, arg_idx)) {
LOG_WRN("(unsigned) char * used for %%p argument. "
"It's recommended to cast it to void * because "
"it may cause misbehavior in certain "
"configurations. String:\"%s\" argument:%d", fmt, arg_idx);
/* Since location is being dropped, decrement
* output length by 2 (argument index + position)
*/
out_len -= 2;
continue;
}
if (is_ro) {
if (flags & CBPRINTF_PACKAGE_CONVERT_RO_STR) {
goto calculate_string_length;
} else {
out_len -= drop_ro_str_pos ? 2 : 1;
}
} else if (flags & CBPRINTF_PACKAGE_CONVERT_RW_STR) {
calculate_string_length:
len = append_string(cb, NULL, str, 0);
/* If possible store calculated string length. */
if (strl && strl_cnt < strl_len) {
strl[strl_cnt++] = (uint16_t)len;
}
out_len += len;
}
if (is_ro && drop_ro_str_pos) {
/* If read-only string location is dropped decreased
* length.
/* string length decremented by 1 because argument
* index is dropped.
*/
out_len--;
out_len += (len - 1);
}
str_pos++;
}
return out_len;
@ -1002,35 +1072,41 @@ int cbprintf_package_convert(void *in_packaged,
* to determine if strings is read-only.
*/
for (int i = 0; i < rws_nbr; i++) {
const char *str = *(const char **)&buf32[*str_pos];
uint8_t arg_idx = *str_pos++;
uint8_t arg_pos = *str_pos++;
const char *str = *(const char **)&buf32[arg_pos];
bool is_ro = ptr_in_rodata(str);
if (fmt_present && is_ptr(fmt, arg_idx)) {
continue;
}
if (is_ro) {
if (flags & CBPRINTF_PACKAGE_CONVERT_RO_STR) {
__ASSERT_NO_MSG(scpy_cnt < sizeof(cpy_str_pos));
cpy_str_pos[scpy_cnt++] = *str_pos;
cpy_str_pos[scpy_cnt++] = arg_pos;
} else if (flags & CBPRINTF_PACKAGE_CONVERT_KEEP_RO_STR) {
__ASSERT_NO_MSG(keep_cnt < sizeof(keep_str_pos));
keep_str_pos[keep_cnt++] = *str_pos;
keep_str_pos[keep_cnt++] = arg_pos;
} else {
/* Drop information about ro_str location. */
}
} else {
if (flags & CBPRINTF_PACKAGE_CONVERT_RW_STR) {
__ASSERT_NO_MSG(scpy_cnt < sizeof(cpy_str_pos));
cpy_str_pos[scpy_cnt++] = *str_pos;
cpy_str_pos[scpy_cnt++] = arg_pos;
} else {
__ASSERT_NO_MSG(keep_cnt < sizeof(keep_str_pos));
keep_str_pos[keep_cnt++] = *str_pos;
keep_str_pos[keep_cnt++] = arg_idx;
keep_str_pos[keep_cnt++] = arg_pos;
}
}
str_pos++;
}
/* Set amount of strings appended to the package. */
out_desc.len = in_desc->len;
out_desc.str_cnt = in_desc->str_cnt + scpy_cnt;
out_desc.rw_str_cnt = (flags & CBPRINTF_PACKAGE_CONVERT_RW_STR) ? 0 : keep_cnt;
out_desc.rw_str_cnt = (flags & CBPRINTF_PACKAGE_CONVERT_RW_STR) ? 0 : (keep_cnt / 2);
out_desc.ro_str_cnt = (flags & CBPRINTF_PACKAGE_CONVERT_RO_STR) ? 0 :
((flags & CBPRINTF_PACKAGE_CONVERT_KEEP_RO_STR) ? keep_cnt : 0);
@ -1055,7 +1131,7 @@ int cbprintf_package_convert(void *in_packaged,
out_len += rv;
/* Copy appended strings from source package to destination. */
size_t strs_len = in_len - (args_size + ros_nbr + rws_nbr);
size_t strs_len = in_len - (args_size + ros_nbr + 2 * rws_nbr);
rv = cb(str_pos, strs_len, ctx);
if (rv < 0) {

41
tests/lib/cbprintf_package/src/main.c
View file

@ -495,14 +495,14 @@ ZTEST(cbprintf_package, test_cbprintf_ro_rw_loc)
zassert_equal(slen, len2);
zassert_equal(memcmp(package, spackage, len), 0);
uint8_t *hdr = package;
struct cbprintf_package_desc *hdr = (struct cbprintf_package_desc *)package;
/* Check that expected number of ro and rw locations are present and no
* strings appended.
*/
zassert_equal(hdr[1], 0);
zassert_equal(hdr[2], 2);
zassert_equal(hdr[3], 2);
zassert_equal(hdr->str_cnt, 0);
zassert_equal(hdr->ro_str_cnt, 2);
zassert_equal(hdr->rw_str_cnt, 2);
int clen;
@ -524,12 +524,12 @@ ZTEST(cbprintf_package, test_cbprintf_ro_rw_loc)
zassert_equal(clen, clen2);
uint8_t *chdr = cpackage;
struct cbprintf_package_desc *chdr = (struct cbprintf_package_desc *)cpackage;
/* Check that read only strings have been appended. */
zassert_equal(chdr[1], 2);
zassert_equal(chdr[2], 0);
zassert_equal(chdr[3], 2);
zassert_equal(chdr->str_cnt, 2);
zassert_equal(chdr->ro_str_cnt, 0);
zassert_equal(chdr->rw_str_cnt, 2);
check_package(package, len, exp_str);
check_package(cpackage, clen, exp_str);
@ -541,8 +541,10 @@ ZTEST(cbprintf_package, test_cbprintf_ro_rw_loc)
clen = cbprintf_package_copy(package, sizeof(package), NULL, 0,
cpy_flags, NULL, 0);
/* Length will be increased by 2 string lengths + null terminators. */
zassert_equal(clen, len + (int)strlen(test_str1) + (int)strlen(test_str2) + 2);
/* Length will be increased by 2 string lengths + null terminators - arg indexes. */
zassert_equal(clen, len + (int)strlen(test_str1) + (int)strlen(test_str2) + 2 - 2,
"exp: %d, got: %d",
clen, len + (int)strlen(test_str1) + (int)strlen(test_str2) + 2 - 2);
uint8_t __aligned(CBPRINTF_PACKAGE_ALIGNMENT) cpackage2[clen];
@ -551,12 +553,12 @@ ZTEST(cbprintf_package, test_cbprintf_ro_rw_loc)
zassert_equal(clen, clen2);
chdr = cpackage2;
chdr = (struct cbprintf_package_desc *)cpackage2;
/* Check that read write strings have been appended. */
zassert_equal(chdr[1], 2);
zassert_equal(chdr[2], 2);
zassert_equal(chdr[3], 0);
zassert_equal(chdr->str_cnt, 2);
zassert_equal(chdr->ro_str_cnt, 2);
zassert_equal(chdr->rw_str_cnt, 0);
check_package(package, len, exp_str);
check_package(cpackage2, clen, exp_str);
@ -736,10 +738,15 @@ static void cbprintf_rw_loc_const_char_ptr(bool keep_ro_str)
clen = cbprintf_package_copy(spackage, sizeof(spackage), NULL, 0,
copy_flags, NULL, 0);
int exp_len = slen + (int)strlen(test_str1) + 1 - (keep_ro_str ? 0 : 1);
/* Previous len + string length + null terminator - argument index -
* decrease size of ro str location. If it is kept then it is decreased
* by 1 (argument index is dropped) if it is discarded then it is decreased
* by 2 (argument index + position dropped).
*/
int exp_len = slen + (int)strlen(test_str1) + 1 - 1 - (keep_ro_str ? 1 : 2);
/* Length will be increased by string length + null terminator. */
zassert_equal(clen, exp_len);
zassert_equal(clen, exp_len, "clen:%d exp_len:%d", clen, exp_len);
uint8_t __aligned(CBPRINTF_PACKAGE_ALIGNMENT) cpackage[clen];
@ -879,7 +886,7 @@ ZTEST(cbprintf_package, test_cbprintf_package_convert)
clen = cbprintf_package_convert(spackage, slen, convert_cb, &ctx, copy_flags, NULL, 0);
zassert_true(clen > 0);
zassert_true(ctx.null);
zassert_equal(ctx.offset, clen);
zassert_equal((int)ctx.offset, clen);
check_package(ctx.buf, ctx.offset, exp_str);
#undef TEST_FMT