[PATCH 1/2] lib/pipeline: bound token concatenation when parsing instructions

[PATCH 1/2] lib/pipeline: bound token concatenation when parsing instructions

[Pengpeng Hou]

action_block_parse() and apply_block_parse() build instruction strings by concatenating untrusted tokens into a fixed-size stack buffer. Replace the open-coded strcat() loop with a helper that tracks the remaining capacity and fails when the assembled instruction would exceed RTE_SWX_INSTRUCTION_SIZE.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 lib/pipeline/rte_swx_pipeline.c      | 30 ++++++++++++---------
 lib/pipeline/rte_swx_pipeline_spec.c | 40 ++++++++++++++++++----------
 2 files changed, 43 insertions(+), 27 deletions(-)

diff --git a/lib/pipeline/rte_swx_pipeline.c b/lib/pipeline/rte_swx_pipeline.c
index a915781..500b9d7 100644
--- a/lib/pipeline/rte_swx_pipeline.c
+++ b/lib/pipeline/rte_swx_pipeline.c
@@ -11,6 +11,7 @@
 #include <rte_eal_memconfig.h>
 #include <rte_jhash.h>
 #include <rte_hash_crc.h>
+#include <rte_string_fns.h>
 
 #include <rte_swx_port_ethdev.h>
 #include <rte_swx_port_fd.h>
@@ -42,6 +43,9 @@ do {                                                                           \
 	       RTE_SWX_INSTRUCTION_SIZE),                                      \
 	      err_code)
 
+#define CHECK_STRLCPY(dst, src, err_code)                                     \
+	CHECK(strlcpy((dst), (src), sizeof(dst)) < sizeof(dst), err_code)
+
 /*
  * Environment.
  */
@@ -5680,7 +5684,7 @@ instr_jmp_translate(struct rte_swx_pipeline *p __rte_unused,
 {
 	CHECK(n_tokens == 2, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	instr->type = INSTR_JMP;
 	instr->jmp.ip = NULL; /* Resolved later. */
@@ -5699,7 +5703,7 @@ instr_jmp_valid_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 3, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	h = header_parse(p, tokens[2]);
 	CHECK(h, EINVAL);
@@ -5722,7 +5726,7 @@ instr_jmp_invalid_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 3, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	h = header_parse(p, tokens[2]);
 	CHECK(h, EINVAL);
@@ -5744,7 +5748,7 @@ instr_jmp_hit_translate(struct rte_swx_pipeline *p __rte_unused,
 	CHECK(!action, EINVAL);
 	CHECK(n_tokens == 2, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	instr->type = INSTR_JMP_HIT;
 	instr->jmp.ip = NULL; /* Resolved later. */
@@ -5762,7 +5766,7 @@ instr_jmp_miss_translate(struct rte_swx_pipeline *p __rte_unused,
 	CHECK(!action, EINVAL);
 	CHECK(n_tokens == 2, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	instr->type = INSTR_JMP_MISS;
 	instr->jmp.ip = NULL; /* Resolved later. */
@@ -5782,7 +5786,7 @@ instr_jmp_action_hit_translate(struct rte_swx_pipeline *p,
 	CHECK(!action, EINVAL);
 	CHECK(n_tokens == 3, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	a = action_find(p, tokens[2]);
 	CHECK(a, EINVAL);
@@ -5806,7 +5810,7 @@ instr_jmp_action_miss_translate(struct rte_swx_pipeline *p,
 	CHECK(!action, EINVAL);
 	CHECK(n_tokens == 3, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	a = action_find(p, tokens[2]);
 	CHECK(a, EINVAL);
@@ -5832,7 +5836,7 @@ instr_jmp_eq_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 4, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	fa = struct_field_parse(p, action, a, &a_struct_id);
 	CHECK(fa, EINVAL);
@@ -5892,7 +5896,7 @@ instr_jmp_neq_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 4, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	fa = struct_field_parse(p, action, a, &a_struct_id);
 	CHECK(fa, EINVAL);
@@ -5952,7 +5956,7 @@ instr_jmp_lt_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 4, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	fa = struct_field_parse(p, action, a, &a_struct_id);
 	CHECK(fa, EINVAL);
@@ -6012,7 +6016,7 @@ instr_jmp_gt_translate(struct rte_swx_pipeline *p,
 
 	CHECK(n_tokens == 4, EINVAL);
 
-	strcpy(data->jmp_label, tokens[1]);
+	CHECK_STRLCPY(data->jmp_label, tokens[1], EINVAL);
 
 	fa = struct_field_parse(p, action, a, &a_struct_id);
 	CHECK(fa, EINVAL);
@@ -6437,7 +6441,7 @@ instr_translate(struct rte_swx_pipeline *p,
 
 	/* Handle the optional instruction label. */
 	if ((n_tokens >= 2) && !strcmp(tokens[1], ":")) {
-		strcpy(data->label, tokens[0]);
+		CHECK_STRLCPY(data->label, tokens[0], EINVAL);
 
 		tpos += 2;
 		CHECK(n_tokens - tpos, EINVAL);
@@ -8541,7 +8545,7 @@ rte_swx_pipeline_table_config(struct rte_swx_pipeline *p,
 	/* Node initialization. */
 	strcpy(t->name, name);
 	if (args && args[0])
-		strcpy(t->args, args);
+		CHECK_STRLCPY(t->args, args, EINVAL);
 	t->type = type;
 
 	for (i = 0; i < params->n_fields; i++) {
diff --git a/lib/pipeline/rte_swx_pipeline_spec.c b/lib/pipeline/rte_swx_pipeline_spec.c
index 986bbe5..3df191e 100644
--- a/lib/pipeline/rte_swx_pipeline_spec.c
+++ b/lib/pipeline/rte_swx_pipeline_spec.c
@@ -39,6 +39,28 @@
 #define LEARNER_TIMEOUT_BLOCK 10
 #define APPLY_BLOCK 11
 
+static int
+buffer_append_tokens(char *buffer, size_t buffer_size, char **tokens,
+		     uint32_t n_tokens)
+{
+	size_t len = 0;
+	uint32_t i;
+
+	buffer[0] = 0;
+	for (i = 0; i < n_tokens; i++) {
+		int ret;
+
+		ret = snprintf(buffer + len, buffer_size - len, "%s%s",
+			       i ? " " : "", tokens[i]);
+		if ((ret < 0) || ((size_t)ret >= buffer_size - len))
+			return -ENAMETOOLONG;
+
+		len += (size_t)ret;
+	}
+
+	return 0;
+}
+
 /*
  * extobj.
  */
@@ -454,7 +476,6 @@ action_block_parse(struct action_spec *s,
 {
 	char buffer[RTE_SWX_INSTRUCTION_SIZE], *instr;
 	const char **new_instructions;
-	uint32_t i;
 
 	/* Handle end of block. */
 	if ((n_tokens == 1) && !strcmp(tokens[0], "}")) {
@@ -463,12 +484,8 @@ action_block_parse(struct action_spec *s,
 	}
 
 	/* spec. */
-	buffer[0] = 0;
-	for (i = 0; i < n_tokens; i++) {
-		if (i)
-			strcat(buffer, " ");
-		strcat(buffer, tokens[i]);
-	}
+	if (buffer_append_tokens(buffer, sizeof(buffer), tokens, n_tokens))
+		return -ENAMETOOLONG;
 
 	instr = strdup(buffer);
 	if (!instr) {
@@ -2142,7 +2159,6 @@ apply_block_parse(struct apply_spec *s,
 {
 	char buffer[RTE_SWX_INSTRUCTION_SIZE], *instr;
 	const char **new_instructions;
-	uint32_t i;
 
 	/* Handle end of block. */
 	if ((n_tokens == 1) && !strcmp(tokens[0], "}")) {
@@ -2151,12 +2167,8 @@ apply_block_parse(struct apply_spec *s,
 	}
 
 	/* spec. */
-	buffer[0] = 0;
-	for (i = 0; i < n_tokens; i++) {
-		if (i)
-			strcat(buffer, " ");
-		strcat(buffer, tokens[i]);
-	}
+	if (buffer_append_tokens(buffer, sizeof(buffer), tokens, n_tokens))
+		return -ENAMETOOLONG;
 
 	instr = strdup(buffer);
 	if (!instr) {
-- 
2.50.1 (Apple Git-155)

Re: [PATCH 1/2] lib/pipeline: bound token concatenation when parsing instructions

[Stephen Hemminger]

On Sat, 21 Mar 2026 10:16:27 +0800
Pengpeng Hou <pengpeng@iscas.ac.cn> wrote:

> action_block_parse() and apply_block_parse() build instruction strings by concatenating untrusted tokens into a fixed-size stack buffer. Replace the open-coded strcat() loop with a helper that tracks the remaining capacity and fails when the assembled instruction would exceed RTE_SWX_INSTRUCTION_SIZE.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---

This patch has serious problems. Finally ran it through AI.


General: these two patches touch unrelated subsystems (lib/pipeline
and net/cpfl), have different maintainers, and are not threaded
(each has its own Message-ID with no In-Reply-To).  They should be
sent as two independent submissions rather than a 1/2, 2/2 series.

Neither patch carries a Fixes: tag or Cc: stable@dpdk.org.  Both
claim to fix buffer overflows in code that shipped years ago, so
both need them.


Patch 1/2: lib/pipeline: bound token concatenation

Error: resource leak in rte_swx_pipeline_table_config()

  CHECK_STRLCPY(t->args, args, EINVAL);

  CHECK() expands to a bare "return -(err_code)".  This call site
  sits after six successful calloc() calls (t, t->fields,
  t->actions, t->default_action_data,
  t->action_is_for_table_entries, t->action_is_for_default_entry).
  Every other failure past "t = calloc(...)" in that function uses
  "goto error", which frees all of them.  A long args string now
  returns -EINVAL and leaks all six allocations.

  This is the concrete hazard in wrapping strlcpy() in a macro that
  hides control flow: the macro is correct in isolation and wrong
  at the only site where it matters.

  The rest of the function validates its string inputs up front
  with CHECK_NAME(), before any allocation.  Doing the same here
  fixes the overflow with no new macro and no leak:

	if (args && args[0])
		CHECK_NAME(args, EINVAL);

  placed next to the existing CHECK_NAME(name, EINVAL), leaving the
  strcpy() in the node-initialization block alone.

Error: err_line / err_msg left unset on the new failure path

  In both action_block_parse() and apply_block_parse():

	if (buffer_append_tokens(buffer, sizeof(buffer), tokens, n_tokens))
		return -ENAMETOOLONG;

  Every other error return in these functions sets *err_line and
  *err_msg before returning.  pipeline_spec_parse() does not
  initialize them either, and callers pass uninitialized locals.
  See examples/pipeline/cli.c:586, which declares

	uint32_t err_line;
	const char *err_msg;

  and on failure does

	snprintf(out, out_size, "Error %d at line %u: %s\n.",
		 status, err_line, err_msg);

  So this path prints an uninitialized pointer through %s.  Set
  both fields, and propagate the helper's return value instead of
  re-hardcoding -ENAMETOOLONG.

Warning: 11 of the 12 strcpy() conversions are dead code

  instr_translate() already validates every token as it is
  tokenized:

	CHECK(n_tokens < RTE_SWX_INSTRUCTION_TOKENS_MAX, EINVAL);
	CHECK_NAME(token, EINVAL);

  CHECK_NAME enforces strnlen(token, RTE_SWX_NAME_SIZE) <
  RTE_SWX_NAME_SIZE, and both instruction_data.label and
  instruction_data.jmp_label are char[RTE_SWX_NAME_SIZE].  No token
  reaching instr_jmp*_translate() or the label handling in
  instr_translate() can overflow, so none of those eleven sites can
  overflow today.  That check was added in commit 0dde44843b
  ("pipeline: fix string copy into fixed size buffer") for exactly
  this class of Coverity report.

  t->args is the one genuinely unchecked destination: it comes
  straight from the public rte_swx_pipeline_table_config() argument
  and from the .spec file table args, with no length validation
  anywhere.  Fixing that one site is worthwhile; the other eleven
  add code without removing a bug.

Warning: commit message describes only half the patch

  The message covers action_block_parse() and apply_block_parse()
  in rte_swx_pipeline_spec.c.  It says nothing about the twelve
  changes to rte_swx_pipeline.c or about the new CHECK_STRLCPY
  macro, which is where the reviewable behaviour change is.

Warning: missing Fixes: and Cc: stable@dpdk.org

  For the strcat() loops:
  Fixes: 3ca60ceed79a ("pipeline: add SWX pipeline specification file")

  For the t->args copy:
  Fixes: e9d870dd93e1 ("pipeline: add SWX pipeline tables")

Info: CHECK_STRLCPY() is fragile if it survives

  sizeof(dst) is silently the pointer size if dst is ever a
  pointer or an array parameter.  All twelve current uses are true
  arrays, so it works, but the file's existing idiom (CHECK_NAME,
  CHECK_INSTRUCTION with an explicit size constant) does not have
  that trap.  If the macro stays, take the size as an explicit
  argument.

Info: the strcat() fix itself is correct

  buffer_append_tokens() is sound: the loop invariant keeps len <
  buffer_size, and the snprintf() return check catches truncation.
  The joined line really can exceed RTE_SWX_INSTRUCTION_SIZE (256),
  since the spec tokenizer accepts lines up to MAX_LINE_LENGTH
  (2048) and up to MAX_TOKENS (256) tokens, so this part of the
  patch fixes a real stack overflow.