On Sat, 21 Mar 2026 10:16:27 +0800 Pengpeng Hou <[email protected]> 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 <[email protected]> > --- 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: [email protected]. 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: [email protected] 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.
