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

[Stephen Hemminger <stephen@networkplumber.org>]

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.