[PATCH iproute2 0/3] hexstring truncation fixes

[PATCH iproute2 0/3] hexstring truncation fixes

[Stephen Hemminger]

This is an alternative to Gris Ge's "Fix netdevsim switch_id display
truncation", which bumped SPRINT_BSIZE from 64 to 128.

hexstring_n2a() writes into a caller buffer and silently truncates when
it is too small: the loop stops once fewer than three bytes remain and
returns the partial string. A 32-byte netdevsim switch id needs 65
bytes but SPRINT_BUF is 64, so "ip -d link show" printed only 62 of the
64 hex characters. Bumping SPRINT_BSIZE hides this one case but leaves
the truncation in place for the next key wider than the buffer, and
enlarges every unrelated SPRINT_BUF on the stack.

Fix the class of bug instead. Add hexstring_alloc(), which sizes the
output to the input, and print_hexstring(), a wrapper over print_string()
that formats and prints a binary attribute with no caller buffer. Move
the print-and-truncate sites onto it, and rebuild the ct label strings,
which open-coded pointer math into a fixed buffer, with hexstring_alloc()
and asprintf().

Stephen Hemminger (3):
  utils: add hexstring_alloc and print_hexstring helpers
  ip, tc: print hex attributes with print_hexstring
  tc: build ct label strings with hexstring_alloc

 include/json_print.h |  3 +++
 include/utils.h      |  1 +
 ip/ipaddress.c       | 19 ++++++-------------
 ip/ipmacsec.c        |  7 ++-----
 lib/bpf_legacy.c     |  6 ++----
 lib/json_print.c     | 12 ++++++++++++
 lib/utils.c          | 21 +++++++++++++++++++++
 tc/f_bpf.c           |  8 +++-----
 tc/f_flower.c        | 25 ++++++++++++++-----------
 tc/m_action.c        |  9 +++------
 tc/m_bpf.c           |  9 +++------
 tc/m_ct.c            | 24 ++++++++++++++----------
 12 files changed, 84 insertions(+), 60 deletions(-)

-- 
2.53.0

[PATCH iproute2 1/3] utils: add hexstring_alloc and print_hexstring helpers

[Stephen Hemminger]

hexstring_n2a() writes into a caller-supplied buffer and silently
truncates when it is too small: the loop stops once fewer than three
bytes remain and returns the partial string. Callers using a fixed
SPRINT_BUF therefore drop trailing bytes for keys wider than 31 bytes.

Add hexstring_alloc(), which formats into a buffer sized to the input
so the full value is always emitted, and print_hexstring(), a wrapper
that formats and prints a binary attribute in one call with no local
buffer to size. print_hexstring() covers the common
print_string(..., hexstring_n2a(...)) pattern.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 include/json_print.h |  3 +++
 include/utils.h      |  1 +
 lib/json_print.c     | 12 ++++++++++++
 lib/utils.c          | 21 +++++++++++++++++++++
 4 files changed, 37 insertions(+)

diff --git a/include/json_print.h b/include/json_print.h
index c0d6315f..6a458189 100644
--- a/include/json_print.h
+++ b/include/json_print.h
@@ -84,6 +84,9 @@ _PRINT_FUNC(float, double)
 _PRINT_FUNC(tv, const struct timeval *)
 #undef _PRINT_FUNC
 
+void print_hexstring(const char *key, const char *fmt,
+		     const __u8 *data, unsigned int len);
+
 #define _PRINT_NAME_VALUE_FUNC(type_name, type, format_char)		  \
 	void print_##type_name##_name_value(const char *name, type value) \
 
diff --git a/include/utils.h b/include/utils.h
index f0d45bad..6a8872bf 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -162,6 +162,7 @@ int get_size64(__u64 *size, const char *str);
 
 int hex2mem(const char *buf, uint8_t *mem, int count);
 char *hexstring_n2a(const __u8 *str, int len, char *buf, int blen);
+char *hexstring_alloc(const __u8 *str, unsigned int len);
 __u8 *hexstring_a2n(const char *str, __u8 *buf, int blen, unsigned int *len);
 #define ADDR64_BUF_SIZE sizeof("xxxx:xxxx:xxxx:xxxx")
 int addr64_n2a(__u64 addr, char *buff, size_t len);
diff --git a/lib/json_print.c b/lib/json_print.c
index 810d496e..21589073 100644
--- a/lib/json_print.c
+++ b/lib/json_print.c
@@ -179,6 +179,18 @@ int print_color_string(enum output_type type,
 	return ret;
 }
 
+/* Print binary data as a hex string. The buffer is sized to the input,
+ * so keys of any length are printed in full rather than truncated.
+ */
+void print_hexstring(const char *key, const char *fmt,
+		     const __u8 *data, unsigned int len)
+{
+	char *hex = hexstring_alloc(data, len);
+
+	print_string(PRINT_ANY, key, fmt, hex ? : "");
+	free(hex);
+}
+
 /*
  * value's type is bool. When using this function in FP context you can't pass
  * a value to it, you will need to use "is_json_context()" to have different
diff --git a/lib/utils.c b/lib/utils.c
index 50602e59..7a1d8b48 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -1186,6 +1186,27 @@ char *hexstring_n2a(const __u8 *str, int len, char *buf, int blen)
 	return buf;
 }
 
+/* Format binary as hex into a freshly allocated string; caller frees.
+ * Unlike hexstring_n2a() the result is always sized to hold the full
+ * input, so there is no silent truncation. Returns NULL on allocation
+ * failure.
+ */
+char *hexstring_alloc(const __u8 *str, unsigned int len)
+{
+	char *buf, *ptr;
+	unsigned int i;
+
+	buf = malloc(2 * len + 1);
+	if (!buf)
+		return NULL;
+
+	for (i = 0, ptr = buf; i < len; i++, ptr += 2)
+		sprintf(ptr, "%02x", str[i]);
+	*ptr = '\0';
+
+	return buf;
+}
+
 __u8 *hexstring_a2n(const char *str, __u8 *buf, int blen, unsigned int *len)
 {
 	unsigned int cnt = 0;
-- 
2.53.0

[PATCH iproute2 2/3] ip, tc: print hex attributes with print_hexstring

[Stephen Hemminger]

Convert the callers that dump a binary attribute straight to output
over to print_hexstring(). This drops the per-site scratch buffer and,
for phys_port_id and phys_switch_id, fixes truncation of keys wider
than 31 bytes: a 32-byte netdevsim switch id previously printed only
62 of its 64 hex characters because it did not fit SPRINT_BUF.

No change to output for keys that already fit.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 ip/ipaddress.c   | 19 ++++++-------------
 ip/ipmacsec.c    |  7 ++-----
 lib/bpf_legacy.c |  6 ++----
 tc/f_bpf.c       |  8 +++-----
 tc/m_action.c    |  9 +++------
 tc/m_bpf.c       |  9 +++------
 6 files changed, 19 insertions(+), 39 deletions(-)

diff --git a/ip/ipaddress.c b/ip/ipaddress.c
index 6017bc83..7e4cb77c 100644
--- a/ip/ipaddress.c
+++ b/ip/ipaddress.c
@@ -1263,22 +1263,15 @@ int print_linkinfo(struct nlmsghdr *n, void *arg)
 				     rta_getattr_str(tb[IFLA_PHYS_PORT_NAME]));
 
 		if (tb[IFLA_PHYS_PORT_ID]) {
-			print_string(PRINT_ANY,
-				     "phys_port_id",
-				     "portid %s ",
-				     hexstring_n2a(
-					     RTA_DATA(tb[IFLA_PHYS_PORT_ID]),
-					     RTA_PAYLOAD(tb[IFLA_PHYS_PORT_ID]),
-					     b1, sizeof(b1)));
+			print_hexstring("phys_port_id", "portid %s ",
+					RTA_DATA(tb[IFLA_PHYS_PORT_ID]),
+					RTA_PAYLOAD(tb[IFLA_PHYS_PORT_ID]));
 		}
 
 		if (tb[IFLA_PHYS_SWITCH_ID]) {
-			print_string(PRINT_ANY,
-				     "phys_switch_id",
-				     "switchid %s ",
-				     hexstring_n2a(RTA_DATA(tb[IFLA_PHYS_SWITCH_ID]),
-						   RTA_PAYLOAD(tb[IFLA_PHYS_SWITCH_ID]),
-						   b1, sizeof(b1)));
+			print_hexstring("phys_switch_id", "switchid %s ",
+					RTA_DATA(tb[IFLA_PHYS_SWITCH_ID]),
+					RTA_PAYLOAD(tb[IFLA_PHYS_SWITCH_ID]));
 		}
 
 		if (tb[IFLA_PARENT_DEV_BUS_NAME]) {
diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index fc4c8631..1864ffd3 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -661,11 +661,8 @@ static void print_flag(struct rtattr *attrs[], const char *desc,
 
 static void print_key(struct rtattr *key)
 {
-	SPRINT_BUF(keyid);
-
-	print_string(PRINT_ANY, "key", " key %s\n",
-		     hexstring_n2a(RTA_DATA(key), RTA_PAYLOAD(key),
-				   keyid, sizeof(keyid)));
+	print_hexstring("key", " key %s\n",
+			RTA_DATA(key), RTA_PAYLOAD(key));
 }
 
 #define CIPHER_NAME_GCM_AES_128 "GCM-AES-128"
diff --git a/lib/bpf_legacy.c b/lib/bpf_legacy.c
index 50ca82c1..9d5fcf5f 100644
--- a/lib/bpf_legacy.c
+++ b/lib/bpf_legacy.c
@@ -176,7 +176,6 @@ int bpf_dump_prog_info(FILE *f, uint32_t id)
 	struct bpf_prog_info info = {};
 	uint32_t len = sizeof(info);
 	int fd, ret, dump_ok = 0;
-	SPRINT_BUF(tmp);
 
 	open_json_object("prog");
 	print_uint(PRINT_ANY, "id", "id %u ", id);
@@ -190,9 +189,8 @@ int bpf_dump_prog_info(FILE *f, uint32_t id)
 		int jited = !!info.jited_prog_len;
 
 		print_string(PRINT_ANY, "name", "name %s ", info.name);
-		print_string(PRINT_ANY, "tag", "tag %s ",
-			     hexstring_n2a(info.tag, sizeof(info.tag),
-					   tmp, sizeof(tmp)));
+		print_hexstring("tag", "tag %s ",
+				info.tag, sizeof(info.tag));
 		print_uint(PRINT_JSON, "jited", NULL, jited);
 		if (jited && !is_json_context())
 			fprintf(f, "jited ");
diff --git a/tc/f_bpf.c b/tc/f_bpf.c
index 6dd75445..50fe01ca 100644
--- a/tc/f_bpf.c
+++ b/tc/f_bpf.c
@@ -241,11 +241,9 @@ static int bpf_print_opt(const struct filter_util *qu, FILE *f,
 	if (tb[TCA_BPF_ID])
 		dump_ok = bpf_dump_prog_info(f, rta_getattr_u32(tb[TCA_BPF_ID]));
 	if (!dump_ok && tb[TCA_BPF_TAG]) {
-		SPRINT_BUF(b);
-
-		print_string(PRINT_ANY, "tag", "tag %s ",
-			     hexstring_n2a(RTA_DATA(tb[TCA_BPF_TAG]),
-			     RTA_PAYLOAD(tb[TCA_BPF_TAG]), b, sizeof(b)));
+		print_hexstring("tag", "tag %s ",
+				RTA_DATA(tb[TCA_BPF_TAG]),
+				RTA_PAYLOAD(tb[TCA_BPF_TAG]));
 	}
 
 	if (tb[TCA_BPF_POLICE]) {
diff --git a/tc/m_action.c b/tc/m_action.c
index 6f79fdae..bbc8e6a6 100644
--- a/tc/m_action.c
+++ b/tc/m_action.c
@@ -401,12 +401,9 @@ static int tc_print_one_action(FILE *f, struct rtattr *arg, bool bind)
 		print_nl();
 	}
 	if (tb[TCA_ACT_COOKIE]) {
-		int strsz = RTA_PAYLOAD(tb[TCA_ACT_COOKIE]);
-		char b1[strsz * 2 + 1];
-
-		print_string(PRINT_ANY, "cookie", "\tcookie %s",
-			     hexstring_n2a(RTA_DATA(tb[TCA_ACT_COOKIE]),
-					   strsz, b1, sizeof(b1)));
+		print_hexstring("cookie", "\tcookie %s",
+				RTA_DATA(tb[TCA_ACT_COOKIE]),
+				RTA_PAYLOAD(tb[TCA_ACT_COOKIE]));
 		print_nl();
 	}
 	if (tb[TCA_ACT_FLAGS] || tb[TCA_ACT_IN_HW_COUNT]) {
diff --git a/tc/m_bpf.c b/tc/m_bpf.c
index a5de7da1..83b21b57 100644
--- a/tc/m_bpf.c
+++ b/tc/m_bpf.c
@@ -183,12 +183,9 @@ static int bpf_print_opt(const struct action_util *au, FILE *f, struct rtattr *a
 		d_ok = bpf_dump_prog_info(f,
 					  rta_getattr_u32(tb[TCA_ACT_BPF_ID]));
 	if (!d_ok && tb[TCA_ACT_BPF_TAG]) {
-		SPRINT_BUF(b);
-
-		print_string(PRINT_ANY, "tag", "tag %s ",
-			     hexstring_n2a(RTA_DATA(tb[TCA_ACT_BPF_TAG]),
-			     RTA_PAYLOAD(tb[TCA_ACT_BPF_TAG]),
-			     b, sizeof(b)));
+		print_hexstring("tag", "tag %s ",
+				RTA_DATA(tb[TCA_ACT_BPF_TAG]),
+				RTA_PAYLOAD(tb[TCA_ACT_BPF_TAG]));
 	}
 
 	print_action_control("default-action ", parm->action, _SL_);
-- 
2.53.0

[PATCH iproute2 3/3] tc: build ct label strings with hexstring_alloc

[Stephen Hemminger]

flower_print_ct_label() and ct_print_labels() formatted the key and
optional mask into a fixed on-stack buffer with manual pointer
arithmetic, relying on hexstring_n2a() not to overrun it. Build the
"key/mask" string with hexstring_alloc() and asprintf() instead, which
sizes to the data and drops the open-coded offset handling.

Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
 tc/f_flower.c | 25 ++++++++++++++-----------
 tc/m_ct.c     | 24 ++++++++++++++----------
 2 files changed, 28 insertions(+), 21 deletions(-)

diff --git a/tc/f_flower.c b/tc/f_flower.c
index 6fc2c6a1..d6844282 100644
--- a/tc/f_flower.c
+++ b/tc/f_flower.c
@@ -2619,18 +2619,15 @@ static void flower_print_ct_label(struct rtattr *attr,
 {
 	const unsigned char *str;
 	bool print_mask = false;
+	char *key, *out;
 	int data_len, i;
-	char out[128];
-	char *p;
 
 	if (!attr)
 		return;
 
 	data_len = RTA_PAYLOAD(attr);
-	hexstring_n2a(RTA_DATA(attr), data_len, out, sizeof(out));
-	p = out + data_len*2;
+	key = hexstring_alloc(RTA_DATA(attr), data_len);
 
-	data_len = RTA_PAYLOAD(attr);
 	str = RTA_DATA(mask_attr);
 	if (data_len != 16)
 		print_mask = true;
@@ -2638,16 +2635,22 @@ static void flower_print_ct_label(struct rtattr *attr,
 		if (str[i] != 0xff)
 			print_mask = true;
 	}
+
 	if (print_mask) {
-		*p++ = '/';
-		hexstring_n2a(RTA_DATA(mask_attr), data_len, p,
-			      sizeof(out)-(p-out));
-		p += data_len*2;
+		char *mask = hexstring_alloc(RTA_DATA(mask_attr), data_len);
+
+		if (asprintf(&out, "%s/%s", key, mask) < 0)
+			out = NULL;
+		free(mask);
+		free(key);
+	} else {
+		out = key;
 	}
-	*p = '\0';
 
 	print_nl();
-	print_string(PRINT_ANY, "ct_label", "  ct_label %s", out);
+	print_string(PRINT_ANY, "ct_label", "  ct_label %s", out ? : "");
+
+	free(out);
 }
 
 static void flower_print_ct_zone(struct rtattr *attr,
diff --git a/tc/m_ct.c b/tc/m_ct.c
index e549cb9c..e5eebbda 100644
--- a/tc/m_ct.c
+++ b/tc/m_ct.c
@@ -443,17 +443,15 @@ static void ct_print_labels(struct rtattr *attr,
 {
 	const unsigned char *str;
 	bool print_mask = false;
-	char out[256], *p;
+	char *key, *out;
 	int data_len, i;
 
 	if (!attr)
 		return;
 
 	data_len = RTA_PAYLOAD(attr);
-	hexstring_n2a(RTA_DATA(attr), data_len, out, sizeof(out));
-	p = out + data_len*2;
+	key = hexstring_alloc(RTA_DATA(attr), data_len);
 
-	data_len = RTA_PAYLOAD(attr);
 	str = RTA_DATA(mask_attr);
 	if (data_len != 16)
 		print_mask = true;
@@ -461,15 +459,21 @@ static void ct_print_labels(struct rtattr *attr,
 		if (str[i] != 0xff)
 			print_mask = true;
 	}
+
 	if (print_mask) {
-		*p++ = '/';
-		hexstring_n2a(RTA_DATA(mask_attr), data_len, p,
-			      sizeof(out)-(p-out));
-		p += data_len*2;
+		char *mask = hexstring_alloc(RTA_DATA(mask_attr), data_len);
+
+		if (asprintf(&out, "%s/%s", key, mask) < 0)
+			out = NULL;
+		free(mask);
+		free(key);
+	} else {
+		out = key;
 	}
-	*p = '\0';
 
-	print_string(PRINT_ANY, "label", " label %s", out);
+	print_string(PRINT_ANY, "label", " label %s", out ? : "");
+
+	free(out);
 }
 
 static void ct_print_helper(struct rtattr *family, struct rtattr *proto, struct rtattr *name)
-- 
2.53.0