Lots of problems found from review of ARK PMD

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: [email protected]
          Reporter: [email protected]

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.