On Thu, 7 May 2026 14:20:11 +0300
Denis Lyulin <[email protected]> wrote:
> When rte_flow_conv_item_spec() is called from rte_flow_conv_pattern(),
> the spec, last and mask pointers are checked separately. If the API
> is used incorrectly, the spec pointer may be NULL while last and mask
> may be valid pointers.
>
> In rte_flow_conv_item_spec() for GENVE_OPT and RAW item types the spec
> pointer is used even if the function is called to copy last or mask.
> It may cause a NULL pointer (spec) dereference.
>
> This commit adds extra check of item->spec and if it is NULL, does not
> copy further data relying on it
>
> Fixes: 841a0445442d ("ethdev: fix GENEVE option item conversion")
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Signed-off-by: Denis Lyulin <[email protected]>
> ---
AI spotted a couple issues here, please address.
Warnings
========
* lib/ethdev/rte_flow.c: RAW case guard is overly broad and regresses
the LAST-without-spec path.
The new wrapper `if (spec.raw && last.raw) { ... } else { tmp = 0; }`
is too coarse. The original code only dereferences spec.raw on the
SPEC and MASK paths:
if (type == RTE_FLOW_CONV_ITEM_SPEC || // uses spec, mask
(type == RTE_FLOW_CONV_ITEM_MASK && // uses spec, last,
mask
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length))))
tmp = spec.raw->length & mask.raw->length;
else
tmp = last.raw->length & mask.raw->length; // uses last, mask
only
For type == RTE_FLOW_CONV_ITEM_LAST the else branch is taken and
spec.raw is never touched. With the patch, when an item has
item->last set but item->spec == NULL, the wrapper short-circuits
to tmp = 0 and the `last` pattern is silently dropped (dst.raw->
pattern stays NULL after the struct-initializer memcpy). The
original code copied it correctly.
rte_flow_conv_pattern() calls this function with type=LAST whenever
src->last is non-NULL, regardless of src->spec, so this path is
reachable through the public conv API.
Note also that mask.raw is guaranteed non-NULL by the
`item->mask ? item->mask : &rte_flow_item_raw_mask` fallback two
lines above, so it does not need to participate in the guard.
A more surgical guard preserves the LAST behaviour, e.g.:
if (type == RTE_FLOW_CONV_ITEM_SPEC && spec.raw)
tmp = spec.raw->length & mask.raw->length;
else if (type == RTE_FLOW_CONV_ITEM_MASK && spec.raw && last.raw &&
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length)))
tmp = spec.raw->length & mask.raw->length;
else if (last.raw)
tmp = last.raw->length & mask.raw->length;
else
tmp = 0;
Info
====
* The commit message attributes the bug only to calls from
rte_flow_conv_pattern(), but the more directly reachable path is
rte_flow_conv() with RTE_FLOW_CONV_OP_ITEM_MASK (rte_flow.c around
line 1144), which validates only item->mask and leaves item->spec
potentially NULL before invoking rte_flow_conv_item_spec() with
type=MASK. Worth mentioning so reviewers can locate the trigger.
* The GENEVE_OPT hunk uses spec.geneve_opt->option_len to size the
copy of src.geneve_opt->data, even when src is the mask (type=MASK)
or the last (type=LAST). This pre-existing assumption (that
spec.option_len describes the layout for all three) is preserved by
the patch and is out of scope here, but worth a sanity check that
the spec.option_len fallback to 0 (no copy) is the desired
behaviour when only a mask is supplied via OP_ITEM_MASK.
