Re: [PATCH] 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 · Medium: 0 · Low: 0

- [High] Incomplete bounds check: prestera_hw_counters_get()
  (drivers/net/ethernet/marvell/prestera/prestera_hw.c:2327) validates…

--

net: prestera: validate num_counters in counter response

This adds a check in prestera_hw_counters_get() that rejects a firmware
counter response whose num_counters field exceeds the number of counters
that were requested, returning -EINVAL instead of iterating over the
response.

> diff --git a/drivers/net/ethernet/marvell/prestera/prestera_hw.c b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> index 7695cbb2ce626..c4cb5f22893c9 100644
> --- a/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> +++ b/drivers/net/ethernet/marvell/prestera/prestera_hw.c
> @@ -2324,6 +2324,11 @@ int prestera_hw_counters_get(struct prestera_switch *sw, u32 idx,
>  	if (err)
>  		goto free_buff;
>  
> +	if (__le32_to_cpu(resp->num_counters) > *len) {
> +		err = -EINVAL;
> +		goto free_buff;
> +	}
> +

[Severity: High]
The commit message says this closes "out-of-bounds writes to the caller
stats array", but does *len actually describe the capacity of stats[]?

The only caller is prestera_counter_stats_work() in prestera_counter.c,
which always requests a fixed chunk size and passes a pointer into the
middle of block->stats:

	u32 count = COUNTER_BULK_SIZE;
	...
	err = prestera_hw_counters_get(counter->sw, counter->total_read,
				       &count, &done,
				       &block->stats[counter->total_read]);

COUNTER_BULK_SIZE is 256, while block->stats is allocated with exactly
block->num_counters elements in prestera_counter_block_get():

	err = prestera_hw_counter_block_get(counter->sw, client,
					    &block->id, &block->offset,
					    &block->num_counters);
	...
	block->stats = kzalloc_objs(*block->stats, block->num_counters);

block->num_counters also comes from the firmware, so it can be smaller
than 256 and need not be a multiple of 256.

Can stats[] still be overflowed with this check in place?  If firmware
reports num_counters = 10 for the block and then answers the
COUNTER_GET with resp->num_counters = 256, the new test (256 > 256) is
false and the loop below writes stats[0..255] into a 10 element array.
Similarly, with block->num_counters = 300, the second poll iteration
passes &block->stats[256] which has only 44 slots left, yet *len is
still 256, so any reply in the range 45..256 passes.

Would clamping the requested count in prestera_counter_stats_work() to
block->num_counters - counter->total_read, or passing the remaining
capacity down to prestera_hw_counters_get(), be a better place for the
bound?  The helper as written never learns the size of the destination
array.

>  	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: High]
Related to the above: on success *len is set to resp->num_counters, and
prestera_counter_stats_work() then does:

	counter->total_read += count;
	if (!done || counter->total_read < block->num_counters) {
		resched_time = COUNTER_RESCHED_TIME;
		goto resched;
	}

Can counter->total_read end up larger than block->num_counters here?  If
so, and the same reply has done == 0, the work is rescheduled and the
next invocation forms &block->stats[counter->total_read], which is
already past the end of the array before any write happens.

The out-of-bounds read of resp->stats[] does look addressed by this
check, since the response buffer is allocated with
struct_size(resp, stats, *len).  Would it be worth splitting the commit
message so it only claims the read side, or extending the fix to cover
the write side too?

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