Change in libosmo-pfcp[master]: gtlv: check memory bounds 3/3: encoding to str - gerrit-log

25 Aug 2022

neels has submitted this change. ( https://gerrit.osmocom.org/c/libosmo-pfcp/+/29041 )
Change subject: gtlv: check memory bounds 3/3: encoding to str
......................................................................
gtlv: check memory bounds 3/3: encoding to str
See Id8d997c9d5e655ff1842ec69eab6c073875c6330
Related: CID#275417
Related: SYS#5599
Change-Id: I63d52a4f5dba32d3a3887dd9c5e42e1695fb2aa3
---
M include/osmocom/gtlv/gtlv_dec_enc.h
M src/libosmo-gtlv/gtlv_dec_enc.c
M src/libosmo-gtlv/gtlv_gen.c
M tests/libosmo-gtlv/gtlv_dec_enc_test.c
M tests/libosmo-gtlv/test_gtlv_gen/gtlv_gen_test.c
M tests/libosmo-gtlv/test_tliv/tliv_test.c
6 files changed, 30 insertions(+), 23 deletions(-)
Approvals:
  neels: Looks good to me, approved

diff --git a/include/osmocom/gtlv/gtlv_dec_enc.h b/include/osmocom/gtlv/gtlv_dec_enc.h
index 2126beb..e671022 100644
--- a/include/osmocom/gtlv/gtlv_dec_enc.h
+++ b/include/osmocom/gtlv/gtlv_dec_enc.h
@@ -191,10 +191,12 @@
    	      unsigned int obj_ofs, const struct osmo_gtlv_coding *ie_coding, osmo_gtlv_err_cb err_cb,
    	      void *err_cb_data, const struct value_string *iei_strs);
-int osmo_gtlvs_encode_to_str_buf(char *buf, size_t buflen, const void *decoded_struct, unsigned int obj_ofs,
-				const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs);
-char *osmo_gtlvs_encode_to_str_c(void *ctx, const void *decoded_struct, unsigned int obj_ofs,
-				const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs);
+int osmo_gtlvs_encode_to_str_buf(char *buf, size_t buflen,
+				 const void *decoded_struct, size_t decoded_struct_size, unsigned int obj_ofs,
+				 const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs);
+char *osmo_gtlvs_encode_to_str_c(void *ctx,
+				 const void *decoded_struct, size_t decoded_struct_size, unsigned int obj_ofs,
+				 const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs);
static inline bool osmo_gtlv_coding_end(const struct osmo_gtlv_coding *iec)
 {
diff --git a/src/libosmo-gtlv/gtlv_dec_enc.c b/src/libosmo-gtlv/gtlv_dec_enc.c
index e1e6d45..58c0405 100644
--- a/src/libosmo-gtlv/gtlv_dec_enc.c
+++ b/src/libosmo-gtlv/gtlv_dec_enc.c
@@ -28,9 +28,6 @@
#include <osmocom/gtlv/gtlv_dec_enc.h>
-/* Reverse offsetof(): return the address of the struct member for a given osmo_gtlv_msg and member ofs_foo value. */
-#define MEMB(M, MEMB_OFS) ((void *)((char *)(M) + (MEMB_OFS)))
-
 #define RETURN_ERROR(RC, TAG_INST, FMT, ARGS...) \
    do {\
    	if (err_cb) { \
@@ -486,19 +483,23 @@
  * \param[in] iei_strs  value_string array to give IEI names in tag headers, or NULL.
  * \return number of characters that would be written if the buffer is large enough, like snprintf().
  */
-int osmo_gtlvs_encode_to_str_buf(char *buf, size_t buflen, const void *decoded_struct, unsigned int obj_ofs,
-				const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs)
+int osmo_gtlvs_encode_to_str_buf(char *buf, size_t buflen,
+				 const void *decoded_struct, size_t decoded_struct_size, unsigned int obj_ofs,
+				 const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs)
 {
    struct osmo_strbuf sb = { .buf = buf, .len = buflen };
-	void *obj = MEMB(decoded_struct, obj_ofs);
+	const void *obj = membof_const(decoded_struct, decoded_struct_size, obj_ofs);
+	size_t obj_maxlen = decoded_struct_size - obj_ofs;
if (!ie_coding)
    	return -ENOTSUP;
for (; !osmo_gtlv_coding_end(ie_coding); ie_coding++) {
-		bool *presence_flag_p = ie_coding->has_presence_flag ? MEMB(obj, ie_coding->presence_flag_ofs) : NULL;
-		unsigned int *multi_count_p = ie_coding->has_count ? MEMB(obj, ie_coding->count_ofs) : NULL;
+		const bool *presence_flag_p = ie_coding->has_presence_flag ?
+			membof_const(obj, obj_maxlen, ie_coding->presence_flag_ofs) : NULL;
+		const unsigned int *multi_count_p = ie_coding->has_count ?
+			membof_const(obj, obj_maxlen, ie_coding->count_ofs) : NULL;
    	unsigned int n;
    	unsigned int i;
@@ -531,12 +532,14 @@
if (ie_coding->nested_ies) {
    			OSMO_STRBUF_PRINTF(sb, "{");
-				OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf, decoded_struct, obj_ofs + memb_ofs,
+				OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf,
+						   decoded_struct, decoded_struct_size, obj_ofs + memb_ofs,
    					   ie_coding->nested_ies, iei_strs);
    			OSMO_STRBUF_PRINTF(sb, " }");
    		} else {
    			if (ie_coding->enc_to_str_func)
-					OSMO_STRBUF_APPEND(sb, ie_coding->enc_to_str_func, MEMB(obj, memb_ofs));
+					OSMO_STRBUF_APPEND(sb, ie_coding->enc_to_str_func,
+							   membof_const(obj, obj_maxlen, memb_ofs));
    			else
    				OSMO_STRBUF_PRINTF(sb, "(enc_to_str_func==NULL)");
    		}
@@ -557,8 +560,10 @@
  * \param[in] iei_strs  value_string array to give IEI names in tag headers, or NULL.
  * \return human readable string.
  */
-char *osmo_gtlvs_encode_to_str_c(void *ctx, const void *decoded_struct, unsigned int obj_ofs,
-				const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs)
+char *osmo_gtlvs_encode_to_str_c(void *ctx,
+				 const void *decoded_struct, size_t decoded_struct_size, unsigned int obj_ofs,
+				 const struct osmo_gtlv_coding *ie_coding, const struct value_string *iei_strs)
 {
-	OSMO_NAME_C_IMPL(ctx, 256, "ERROR", osmo_gtlvs_encode_to_str_buf, decoded_struct, obj_ofs, ie_coding, iei_strs)
+	OSMO_NAME_C_IMPL(ctx, 256, "ERROR", osmo_gtlvs_encode_to_str_buf, decoded_struct, decoded_struct_size,
+			 obj_ofs, ie_coding, iei_strs)
 }
diff --git a/src/libosmo-gtlv/gtlv_gen.c b/src/libosmo-gtlv/gtlv_gen.c
index 3b2b1f0..e374ea9 100644
--- a/src/libosmo-gtlv/gtlv_gen.c
+++ b/src/libosmo-gtlv/gtlv_gen.c
@@ -397,7 +397,7 @@
    	"int %s_ies_encode_to_str(char *buf, size_t buflen, const union %s_ies *src,\n"
    	"	%s message_type, const struct value_string *iei_strs)\n"
    	"{\n"
-		"	return osmo_gtlvs_encode_to_str_buf(buf, buflen, src, 0, %s_get_msg_coding(message_type), iei_strs);\n"
+		"	return osmo_gtlvs_encode_to_str_buf(buf, buflen, src, sizeof(*src), 0, %s_get_msg_coding(message_type), iei_strs);\n"
    	"}\n",
    	g_cfg->proto_name, g_cfg->proto_name, g_cfg->message_type_enum ? : "int", g_cfg->proto_name);
 }
diff --git a/tests/libosmo-gtlv/gtlv_dec_enc_test.c b/tests/libosmo-gtlv/gtlv_dec_enc_test.c
index a542cfa..f0de7b0 100644
--- a/tests/libosmo-gtlv/gtlv_dec_enc_test.c
+++ b/tests/libosmo-gtlv/gtlv_dec_enc_test.c
@@ -286,7 +286,7 @@
char *decoded_msg_to_str(const struct decoded_msg *m)
 {
-	return osmo_gtlvs_encode_to_str_c(ctx, m, 0, msg_ie_coding, tag_names);
+	return osmo_gtlvs_encode_to_str_c(ctx, m, sizeof(*m), 0, msg_ie_coding, tag_names);
 }
diff --git a/tests/libosmo-gtlv/test_gtlv_gen/gtlv_gen_test.c b/tests/libosmo-gtlv/test_gtlv_gen/gtlv_gen_test.c
index ef5372c..1997fdc 100644
--- a/tests/libosmo-gtlv/test_gtlv_gen/gtlv_gen_test.c
+++ b/tests/libosmo-gtlv/test_gtlv_gen/gtlv_gen_test.c
@@ -203,8 +203,8 @@
 {
    struct osmo_strbuf sb = { .buf = buf, .len = buflen };
    OSMO_STRBUF_PRINTF(sb, "%s={", get_value_string(myproto_msg_type_names, m->type));
-	OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf, &m->ies, 0, myproto_get_msg_coding(m->type),
-			   myproto_iei_names);
+	OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf, &m->ies, sizeof(m->ies), 0,
+			   myproto_get_msg_coding(m->type), myproto_iei_names);
    OSMO_STRBUF_PRINTF(sb, " }");
    return sb.chars_needed;
diff --git a/tests/libosmo-gtlv/test_tliv/tliv_test.c b/tests/libosmo-gtlv/test_tliv/tliv_test.c
index fd4e310..49ae25c 100644
--- a/tests/libosmo-gtlv/test_tliv/tliv_test.c
+++ b/tests/libosmo-gtlv/test_tliv/tliv_test.c
@@ -101,8 +101,8 @@
 {
    struct osmo_strbuf sb = { .buf = buf, .len = buflen };
    OSMO_STRBUF_PRINTF(sb, "%s={", get_value_string(myproto_msg_type_names, m->type));
-	OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf, &m->ies, 0, myproto_get_msg_coding(m->type),
-			   myproto_iei_names);
+	OSMO_STRBUF_APPEND(sb, osmo_gtlvs_encode_to_str_buf, &m->ies, sizeof(m->ies), 0,
+			   myproto_get_msg_coding(m->type), myproto_iei_names);
    OSMO_STRBUF_PRINTF(sb, " }");
    return sb.chars_needed;
null-- 
To view, visit https://gerrit.osmocom.org/c/libosmo-pfcp/+/29041
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmo-pfcp
Gerrit-Branch: master
Gerrit-Change-Id: I63d52a4f5dba32d3a3887dd9c5e42e1695fb2aa3
Gerrit-Change-Number: 29041
Gerrit-PatchSet: 5
Gerrit-Owner: neels nhofmeyr@sysmocom.de
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge laforge@osmocom.org
Gerrit-Reviewer: neels nhofmeyr@sysmocom.de
Gerrit-Reviewer: pespin pespin@sysmocom.de
Gerrit-MessageType: merged

    