[DPDK/ethdev Bug 1950] Lots of problems found from review of ARK PMD

[bugzilla@dpdk.org]

http://bugs.dpdk.org/show_bug.cgi?id=1950

            Bug ID: 1950
           Summary: Lots of problems found from review of ARK PMD
           Product: DPDK
           Version: 22.03
          Hardware: All
                OS: All
            Status: UNCONFIRMED
          Severity: major
          Priority: Normal
         Component: ethdev
          Assignee: dev@dpdk.org
          Reporter: stephen@networkplumber.org
  Target Milestone: ---

In addition to the problems identified by LF scan, using Claude Opus found a
lot more in this driver:

Component: drivers/net/ark
Version: main

Manual review of the full ark driver tree (ark_ethdev.c, ark_ethdev_rx.c,
ark_ethdev_tx.c, ark_pktgen.{c,h}, ark_pktchkr.{c,h}, ark_mpu.c, ark_pktdir.c)
turned up the issues below, in addition to existing scanner reports (strncat
length-arg misuse and non-NUL-terminating strncpy in process_file_args;
unchecked rte_zmalloc_socket before memcpy in ark_dev_init).

== SECURITY (suggest separate tracking item) ==

S1. Unvalidated dlopen() driver overload in check_for_ext() (ark_ethdev.c)
    Loads an arbitrary .so from the ARK_EXT_PATH environment variable
    (dlopen(path, RTLD_LOCAL | RTLD_LAZY)) and resolves ~12 rte_pmd_ark_*
    symbols that are cast to function pointers and called on the control and
    data paths. No path validation, allow-list, ownership/permission check, or
    integrity check. DPDK apps commonly run privileged, so any process able to
    influence the environment gets arbitrary in-process code execution -- a
    local code-injection / privesc vector. RTLD_LAZY defeats up-front symbol
    validation; unchecked dlsym casts are UB on ABI mismatch.
    Recommendation: gate behind a build option off by default; refuse to load
    when privileged or when the file is not root-owned / is world-writable;
    document as debug-only.

== HIGH - crashes / memory safety ==

1. NULL deref in ark_pktgen_setup() (ark_pktgen.c): pktgen toptions[] has no
   "port" entry (only pktchkr's does), but the run branch calls
   options("port")->v.INT; options() returns NULL on miss. Crashes whenever
   Pkt_gen config sets run=1.

2. ark_tx_queue_release() missing NULL guard (ark_ethdev_tx.c): ark_dev_close()
   calls it for every tx_queues[] slot and ark_dev_stop() loops over
   ark_tx_queue_stop() without checks; both deref queue->ddm / cons_index. RX
   side guards (queue == 0); TX side does not -> NULL deref on close/stop with
   any un-setup TX slot.

3. ark_dev_rx_queue_count() missing NULL guard (ark_ethdev_rx.c): dereferences
   queue with no queue == 0 check, unlike every other entry point in the file.

4. mac_addrs alloc failure not handled (ark_ethdev.c ark_dev_init, ~L381):
   failure is logged but execution falls through with mac_addrs == NULL; the
   secondary-port path (~L461) uses goto error, the primary does not.

5. error: path leaks secondary ports (ark_ethdev.c ark_dev_init, ~L480): only
   the primary mac_addrs is freed; already-probed secondary eth_devs and their
   dev_private/mac_addrs leak.

== HIGH - silent data corruption ==

6. OTLONG written through wrong union member (ark_pktgen.c pmd_set_arg,
   ark_pktchkr.c set_arg): "case OTLONG: o->v.INT = atoll(val);" truncates to
   32 bits and leaves stale upper bits in .LONG. Affects num_pkts,
   src_mac_addr, dst_mac_addr. Should be o->v.LONG.

7. src_mac_addr / num_pkts read via .v.INT in setup (both *_setup()): these are
   OTLONG; reading .INT drops upper MAC bytes (default 0xdC3cF6425060).
Adjacent
   dst_mac_addr correctly uses .LONG.

== MEDIUM ==

8. parse_ipv4_string() return type (ark_pktgen.c, ark_pktchkr.c): declared
   int32_t but builds values up to 0xFFFFFFFF via unsigned math; default
   169.254.10.240 goes negative on conversion. Should be uint32_t. "return 0"
   on parse failure also aliases a valid 0.0.0.0.

9. Divide-by-zero risk in ark_api_num_queues_per_port() (ark_mpu.c):
   num_queues / ark_ports; ark_ports derives from untrusted
   user_ext.dev_get_port_count(), unchecked for 0.

10. pkt_size_min > pkt_size_max defaults: 2006/2005 vs 1514 in both option
    tables; likely a typo.

11. dst_port / src_port default 65536: out of range for a 16-bit port.

== LOW / cleanup ==

12. Misplaced log in ark_pktchkr_wait_done(): "pktgen done" prints every loop
    iteration instead of once after completion.

13. ark_pktchkr_get_pkts_sent() returns int for a uint32_t register
    (signedness); pktgen version returns uint32_t.

14. *_parse() tokenizer/doc mismatch: comments document comma-separated args;
    toks[] omits ','.

15. inst->dev_info never set/used in ark_pkt_gen_inst / ark_pkt_chkr_inst
    (allocated via rte_malloc, not zmalloc).

16. check_for_ext() "found" is dead: never reassigned; only the -1
    dlopen-failure path is meaningful.

17. ark_mpu_configure() logs ring_size (uint32_t) with %d.

== PROCESS / TOOLING ==

P1. Non-inclusive naming not caught by review tooling: en_slaved_start
    (ark_pktgen.c toptions[] and the ark_pktgen_set_pkt_ctrl() parameter) uses
    terminology DPDK has been retiring. The substantive point is that
    checkpatches.sh / the checkpatch master|slave test should have flagged this
    and did not -- indicating the file was either grandfathered in or the check
    is not being run against this tree. Coverage gap in the tooling, not just a
    naming nit. Suggest renaming (e.g. en_gated_start) and reviewing why the
    automated check missed it.

-- 
You are receiving this mail because:
You are the assignee for the bug.