Re: [PATCH v2 00/14] common/sfc_efx/base: fix code analysis issues

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 12 Aug 2026 21:08:20 +0400
Ivan Malov <ivan.malov@arknetworks.am> wrote:

> This series addresses code analysis defects in the
> common/sfc_efx/base library.
> 
> The first four patches fix excessive stack consumption in
> MCDI helper functions, each exceeding 1 KB on-stack, by
> switching to heap-allocated payload buffers.
> 
> The remaining ten patches correct SAL annotations, add NULL
> checks across netport and filter helpers, resolving
> uninitialised memory, buffer overrun, and potential
> dereference issues. The final patch widens loop
> variable types to address a CodeQL warning.
> 
> 
> v2:
> 
> - note for the future AI reviews: apply this on top of
>   the 'next-net-main' branch


Still has AI review issues.

Reviewed v2 applied on c1a46b9 ("doc: remove unreferenced KNI and
examples figures"). All 14 apply cleanly. Comparing commit contents
against v1, only patches 11 and 13 have real changes; 12 differs only
in hunk offsets.

Addressed since v1, all correct as far as I can tell:

 - 11/14 now reads MORE_ENTRIES with MCDI_OUT_DWORD_FIELD against
   MAC_STATISTICS_DESCRIPTOR_OUT_FLAGS, so only LBN 0 is tested.
 - 13/14 adds matched_mask so *enum_hwp is written only on the first
   match per SW flag. That restores the original selection order that
   the removed "mask_sw &= ~(flag_sw)" used to provide, including the
   case where several distinct SW flags are set.
 - 13/14 passes cap_enum_hw rather than MC_CMD_FEC_AUTO as the default,
   so a request with no FEC bits keeps MC_CMD_FEC_NONE.
 - __success() placement is now consistent between 09/14 and 13/14.

Patch 11/14: common/sfc_efx/base: fix flex array in netport stat describe

Error: count and stride are still used to index the response buffer with
no bound derived from the response length. This was the main finding on
v1 and is unchanged:

	stride = MCDI_OUT_DWORD(req, MAC_STATISTICS_DESCRIPTOR_OUT_ENTRY_SIZE);
	count = MCDI_OUT_DWORD(req, MAC_STATISTICS_DESCRIPTOR_OUT_ENTRY_COUNT);
	...
		for (i = 0; i < count; ++i) {
			efx_np_stat_describe(entries + i * stride,

Both fields come from firmware. entries points at payload + 20 in a
1020-byte allocation and efx_np_stat_describe() reads 8 bytes per entry,
so any count above (out_sz - 20) / stride reads bytes that were never
written, and count * stride above 1000 reads past the end of the
allocation. The old ENTRIES_NUM(out_sz) expression was wrong for
stride > 8, as the commit message says, but it did bound the loop by the
data actually received; nothing replaces that bound.

	if (stride < MC_CMD_STAT_DESC_LEN ||
	    count > (out_sz -
		     MC_CMD_MAC_STATISTICS_DESCRIPTOR_OUT_ENTRIES_OFST) /
		    stride) {
		rc = EMSGSIZE;
		goto fail4;
	}

The stride test has to come first, otherwise stride == 0 divides by
zero.

Patch 13/14: common/sfc_efx/base: rework SW mask to HW enum converter

Warning: In the fixed-link branch the programmed technology still
changes. link_tech is initialised to MC_CMD_ETH_TECH_NONE and previously
stayed NONE when no requested tech bit was present in the map; passing
MC_CMD_ETH_TECH_AUTO as enum_hw_def now overwrites it. Unlike the FEC
call, which v2 changed to pass the pre-computed value, this one keeps
the hardcoded default. That may well be the intent, but the commit
message is unchanged from v1 and still describes the patch only as a
refactor plus annotation fix; it does not mention the new enum_hw_def
parameter or this behaviour change. Please say so in the commit message,
or pass link_tech to keep the old value.

Patch 01-04: common/sfc_efx/base: reduce stack in ...

Info: Unchanged from v1, repeating for the record. The four conversions
open-code MAX(IN_LEN, OUT_LEN) where EFX_MCDI_BUF_SIZE() exists and also
rounds up to a dword multiple and enforces a two-dword minimum. The
rounding matters because ef10_mcdi_send_request() reads the payload a
full dword at a time. All four current lengths are dword multiples so
there is no defect today, but the property is lost for future length
changes.

Patch 05/14: common/sfc_efx/base: fix filter saved spec handling

Info: Unchanged from v1. Both added NULL checks are unreachable:
saved_spec == NULL forces EF10_FILTER_ADD_NEW in
ef10_filter_add_select_action(), so ADD_STORE and ADD_REPLACE both imply
a non-NULL saved_spec. The __in_opt annotations are right; the STORE
branch already asserts its sibling invariant one line above, so
EFSYS_ASSERT(saved_spec != NULL) would match local style rather than
silently skipping the efs_overridden_spec assignment.

Patch 09/14 and 13/14

Info: The two are consistent with each other now, but both put
__success() on its own line above "static". Existing uses in the tree
put it on the return type line, e.g. ef10_nvram.c:941

	__checkReturn	__success(return != B_FALSE)	boolean_t
	ef10_nvram_buffer_find_item(