Re: [PATCH] ethdev: fix pointer check in GENEVE and RAW flow copy

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu,  7 May 2026 14:20:11 +0300
Denis Lyulin <lyulin.2003@mail.ru> 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: michaelba@nvidia.com
> Cc: adrien.mazarguil@6wind.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Denis Lyulin <lyulin.2003@mail.ru>
> ---

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.