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

[netdev-bot+sashiko@kernel.org]

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.

> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_counter.c b/drivers/net/ethernet/marvell/prestera/prestera_counter.c
> index 3a762d17ba6c6..f261104729e91 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_counter.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_counter.c
> @@ -342,6 +342,7 @@ static void prestera_counter_stats_work(struct work_struct *work)
>  	u32 resched_time = COUNTER_POLL_TIME;
>  	u32 count = COUNTER_BULK_SIZE;
>  	bool done = false;
> +	u32 remaining;
>  	int err;
>  	u32 i;
>  
> @@ -368,10 +369,12 @@ static void prestera_counter_stats_work(struct work_struct *work)
>  		goto resched;
>  	}
>  
> +	remaining = 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.

> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> index 7695cbb2ce626..2e5cffefea701 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> @@ -2324,6 +2325,11 @@ int prestera_hw_counters_get(struct prestera_switch *sw, u32 idx,
>  	if (err)
>  		goto free_buff;
>  
> +	if (__le32_to_cpu(resp->num_counters) > min(*len, stats_len)) {
> +		err = -EINVAL;
> +		goto free_buff;
> +	}
> +
>  	for (i = 0; i < __le32_to_cpu(resp->num_counters); i++) {
>  		stats[i].packets += __le64_to_cpu(resp->stats[i].packets);
>  		stats[i].bytes += __le64_to_cpu(resp->stats[i].bytes);

[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