Re: [PATCH net v2] net: prestera: validate num_counters in counter response

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

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

- [High] In prestera_counter_stats_work() the new capacity value is
  computed as an unguarded u32 subtraction, `remaining =…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but
  block->counter_flag, allocated with kcalloc(block->num_counters, 1) in…
- [Medium] prestera_hw_counters_get() derives resp->num_counters,
  resp->done and every resp->stats[i] entry from a buffer allocated with…

--

net: prestera: validate num_counters in counter response

The patch adds a stats_len parameter to prestera_hw_counters_get() and
rejects a firmware reply whose num_counters exceeds either the requested
bulk count or the space left in the caller's stats array.  The only caller,
prestera_counter_stats_work(), computes that space as
block->num_counters - counter->total_read.

[Severity: High]
Can this u32 subtraction underflow, and if so does the new bound still
protect block->stats[]?

counter->total_read and counter->is_fetching live in struct
prestera_counter and survive across work invocations, but the block
reference is dropped on every reschedule:

  resched:
  	if (block)
  		prestera_counter_block_put(counter, block);
  	schedule_delayed_work(&counter->stats_dw, resched_time);

If the last counter of that block is released in the meantime, the
refcount-zero path in prestera_counter_block_put() clears the list slot
and frees the block:

  	for (i = 0; i < counter->block_list_len; i++) {
  		if (counter->block_list[i] &&
  		    counter->block_list[i]->id == block->id) {
  			counter->block_list[i] = NULL;
  			break;
  		}
  	}

prestera_counter_block_list_add() then reuses the first NULL slot, so a
different block, with its own firmware-reported num_counters, can end up
at the same index.  prestera_counter_block_get_by_idx() re-resolves purely
by index:

  	if (idx >= counter->block_list_len)
  		return NULL;
  	...
  	if (!counter->block_list[idx] ||
  	    !prestera_counter_block_incref(counter->block_list[idx])) {

There is no check that this is the same block the in-progress fetch
started on, so the next poll continues with the old total_read.  When
total_read is larger than the new block->num_counters, remaining wraps to
near 4G and &block->stats[counter->total_read] is already past the end of
the smaller allocation.

Should total_read be validated or clamped against block->num_counters
here, or should the fetch state (is_fetching/total_read) be made
per-block, rather than deriving the bound from it?

 	prestera_counter_block_lock(block);
 	err = prestera_hw_counters_get(counter->sw, counter->total_read,
 				       &count, &done,
-				       &block->stats[counter->total_read]);
+				       &block->stats[counter->total_read],
+				       remaining);
 	prestera_counter_block_unlock(block);
 	if (err)
 		goto abort;

The commit message says:

    Pass the remaining capacity down and reject a reply that exceeds the
    request or the space left. This also keeps counter->total_read within
    block->num_counters, so the next poll cannot start past the array.

Does that invariant hold only while the block instance at curr_idx stays
the same?  If the block can be replaced between polls, the "next poll
cannot start past the array" claim looks like it needs qualifying.

[Severity: Medium]
This isn't a bug introduced by this patch, but the new check relates
resp->num_counters to the requested count and the destination capacity,
never to the number of bytes the device actually wrote.

The reply buffer in prestera_hw_counters_get() comes from kmalloc(), not
kzalloc(), and prestera_fw_cmd_send() only bounds the reply length from
above and copies exactly that many bytes:

  drivers/net/ethernet/marvell/prestera/prestera_pci.c:prestera_fw_cmd_send() {
  	ret_size = prestera_fw_read(fw, PRESTERA_CMDQ_RCV_LEN_REG(qid));
  	if (ret_size > out_size) {
  	...
  	memcpy_fromio(out_msg, prestera_fw_cmdq_buf(fw, qid) + in_size, ret_size);
  }

ret_size is not propagated back, and prestera_cmd_ret() validates only the
ACK type and status, which sit in the first bytes of the reply.  For a
short but ACKed reply, num_counters, done and stats[] would be stale heap
content, and a garbage count within min(*len, stats_len) passes the new
test, so uninitialised bytes get accumulated with += into block->stats[]
and later reported to userspace via prestera_counter_stats_get() ->
prestera_acl_rule_get_stats().

Would using kzalloc() for the reply buffer, or plumbing ret_size out of
the send path so the reply length can be checked against
struct_size(resp, stats, resp->num_counters), be worth doing while this
area is being touched?

[Severity: High]
This is a pre-existing issue and not introduced by this patch, but since
it is in the same block lifecycle the patch relies on: does
prestera_counter_block_put() leak block->counter_flag?

prestera_counter_block_get() allocates it:

  	block->counter_flag = kcalloc(block->num_counters,
  				      sizeof(*block->counter_flag),
  				      GFP_KERNEL);

and the refcount-zero teardown frees everything else:

  	prestera_hw_counter_block_release(counter->sw, block->id);
  	idr_destroy(&block->counter_idr);
  	mutex_destroy(&block->mtx);
  	kfree(block->stats);
  	kfree(block);

The only kfree(block->counter_flag) in the driver is on the err_list_add
unwind path in prestera_counter_block_get(), so every block create/destroy
cycle driven by tc flower rule add/delete appears to lose num_counters
bytes.  Should a kfree(block->counter_flag) be added next to
kfree(block->stats)?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001EE76C8EC41B2CEFAE213C8BB2%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM