Re: [PATCH net-next 07/10] ice: parser: use kcalloc for table allocation

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] ice_parser_create_table() still allows a heap out-of-bounds
  write controlled by firmware data.
- [Medium] The commit message says kcalloc() is used so that "an overflow
  in the firmware-provided item dimensions is detected before…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a high-
  severity bug on the same allocation-failure path.
- [Medium] This problem wasn't introduced by this patch.

--

ice: parser: use kcalloc for table allocation

This commit changes the allocation in ice_parser_create_table() from
kzalloc(item_size * length) to kcalloc(length, item_size). The goal is
to catch multiplication overflow before the allocation.

Use kcalloc() when calculating the parser table allocation size so an
overflow in the firmware-provided item dimensions is detected before
allocation.

[Severity: Medium]
Is this description accurate? In ice_parser_create_table(), neither
item_size nor length comes from firmware.

All of the *_table_get() callers pass a sizeof() and an ICE_*_TABLE_SIZE
constant. For example, ice_pg_cam_table_get() does this:

	return ice_parser_create_table(hw, ICE_SID_RXPARSER_CAM,
				       sizeof(struct ice_pg_cam_item),
				       ICE_PG_CAM_TABLE_SIZE,

The largest length is ICE_PG_CAM_TABLE_SIZE (2048). The multiplication
cannot overflow, so the kcalloc() overflow check can never fire.

Could the commit message describe this as a cleanup, not as hardening
against firmware input?

[Severity: High]
The table holds length entries. Can the fill loop right after this still
write past the end of it?

In ice_parser_create_table(), the index comes from the package section
header, and nothing compares it with length:

		if (data) {
			struct ice_pkg_sect_hdr *hdr = state.sect;

			if (!no_offset)
				idx = le16_to_cpu(hdr->offset) +
					state.entry_idx;

			item = (void *)((uintptr_t)table + idx * item_size);
			parse_item(hw, idx, item, data, item_size);

			if (no_offset)
				idx++;
		}

ice_parser_sect_item_get() only checks the entry index against the
package's own hdr->count. In no_offset mode (bst_tcam, bst_lbl,
ptype_mk_tcam), idx goes up once for every entry in every section of
that type, with no limit.

For example, the flg_rd table has ICE_FLG_RD_TABLE_SIZE (64) entries. A
section with count = 1 and offset = 64 would make ice_flg_rd_parse_item()
write to table[64]:

	rdi->idx = idx;
	rdi->expose = FIELD_GET(ICE_FRT_EXPO, d8);
	rdi->intr_flg_id = FIELD_GET(ICE_FRT_IFID, d8);

The source data is hw->seg, which comes from the DDP package in
/lib/firmware. A VF request can trigger the table fill at runtime:

ice_vc_fdir_parse_raw() / ice_parse_raw_rss_pattern()
  ice_parser_create()
    ice_flg_rd_table_get()
      ice_parser_create_table()

The commit message is about guarding this table against firmware input.
Would it make sense to compute idx in a type wide enough not to wrap, and
to reject or skip entries with idx >= length before computing item and
calling parse_item()?

 	if (!table)
 		return ERR_PTR(-ENOMEM);

[Severity: High]
This isn't a bug introduced by this patch, but what happens to this
ERR_PTR(-ENOMEM) once it gets back to ice_parser_create()?

Each sub-table result is stored in the struct before the IS_ERR() check,
and the field is not cleared before the jump to the error label:

	p->imem_table = ice_imem_table_get(hw);
	if (IS_ERR(p->imem_table)) {
		err = p->imem_table;
		goto err;
	}
	...
err:
	ice_parser_destroy(p);
	return err;

ice_parser_destroy() then calls kfree() on every field without checking
IS_ERR():

	kfree(psr->imem_table);
	kfree(psr->mi_table);
	...

kfree() only returns early for ZERO_OR_NULL_PTR(). An ERR_PTR value goes
on to virt_to_page() and the slab free path with a bogus address.

A failed kcalloc() here can cause this. The pg_cam table has 2048
entries, so that is a multi-page GFP_KERNEL allocation. Other sources
are ERR_PTR(-EINVAL) when hw->seg is NULL, and ice_xlt_kb_get()
returning ERR_PTR(-EINVAL) for a missing key builder section or
ERR_PTR(-ENOMEM) when its kzalloc fails.

ice_parser_create() can be reached from VF raw FDIR and RSS requests.
Can a VF keep triggering an invalid free on the host this way?

Would it help to keep each result in a local variable and assign it only
on success, or to set the field to NULL before goto err?

[Severity: Medium]
This is a pre-existing issue, but can the data pointer passed to
parse_item() point past the end of the package section?

ice_parser_sect_item_get() only checks the index against the section's
own count:

	hdr = section;
	if (index >= le16_to_cpu(hdr->count))
		return NULL;

	return section + data_off + index * size;

ice_pkg_enum_section() checks that the section's offset and size fit in
ICE_PKG_BUF_SIZE. Nothing checks that data_off + count * size fits
inside the section.

Counts can be as high as 65535, and entries as large as
ICE_SID_LBL_ENTRY_SIZE bytes. With a large count, the returned pointers
would go past the 4 KB package buffer. Each parse_item callback then
reads size bytes from there into the parser tables.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929224153.1455466-1-anthony.l.nguyen%40intel.com