Re: [PATCH] net: ncsi: clamp filter table counts in GP response handling

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 (1) · Medium: 2 (2) · 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] The fix is incomplete, and the commit message is wrong about the
  SVF/SMA handlers.
- [High] The new limits in ncsi_rsp_handler_gp() assume n_uc+n_mc+n_mixed
  and n_vids always describe the buffers behind ncmf->addrs and…
- [Medium] The new limit on the MAC loop in ncsi_rsp_handler_gp()
  (net/ncsi/ncsi-rsp.c:886-888) also limits how far the wire cursor…
- [Medium] ncsi_rsp_handler_gp() now reads n_uc+n_mc+n_mixed and n_vids
  under nc->lock as the size of ncmf->addrs / ncvf->vids.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the loops it
  changes still read past the end of the received GP response.
- [Medium] This problem wasn't introduced by this patch, but the new
  limit of 64 (sizeof(bitmap)*8) does not match the width of the on-wire…
- [Medium] This problem wasn't introduced by this patch, but
  ncsi_rsp_handler_gc() (net/ncsi/ncsi-rsp.c:816 and 823) assigns new…

--

net: ncsi: clamp filter table counts in GP response handling

This patch bounds the MAC and VLAN filter table loops in
ncsi_rsp_handler_gp(). The new bounds are the entry counts allocated by the
Get Capabilities handler and the width of the u64 filter bitmaps. The goal
is to stop a device that reports inconsistent GC and GP counts from causing
heap out-of-bounds writes and out-of-bounds bit operations.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a..fa1ed405dfbb 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -883,7 +883,9 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  	ncmf = &nc->mac_filter;
>  	spin_lock_irqsave(&nc->lock, flags);
>  	bitmap = &ncmf->bitmap;
> -	for (i = 0; i < rsp->mac_cnt; i++, pdata += 6) {
> +	for (i = 0; i < rsp->mac_cnt &&
> +	     i < ncmf->n_uc + ncmf->n_mc + ncmf->n_mixed &&
> +	     i < sizeof(ncmf->bitmap) * 8; i++, pdata += 6) {

[Severity: Medium]
When this loop exits early, is pdata still inside the MAC table when the
VLAN loop starts?

In the GP response the VLAN tag table starts at
rsp + 48 + rsp->mac_cnt * 6. With this change, pdata only moves forward for
the MAC entries that are actually stored.

Take the example from the commit message: GC advertises one unicast filter
and GP reports mac_cnt = 255. The MAC loop stops after one entry and leaves
pdata at rsp + 54. The VLAN loop then runs:

	ncvf->vids[i] = ntohs(*(__be16 *)pdata);

so it stores MAC table bytes as VLAN IDs. The enable bits still come from
the correctly located rsp->vlan_enable.

Those bogus vids[] values are used later:

  set_one_vid() skips a VID it already finds in vids[], so a real VID may
  never be programmed

  clear_one_vid() sends SVF commands for the bogus VIDs

  NCSI_CMD_PKG_INFO reports them to userspace

Could the patch limit only the number of stored entries, and reset the
cursor before the VLAN loop? For example:

	pdata = (unsigned char *)rsp + 48 + rsp->mac_cnt * ETH_ALEN;

[Severity: High]
This bound assumes that n_uc + n_mc + n_mixed always describes the buffer
behind ncmf->addrs. The commit message calls it "the number of entries
actually allocated". Does that hold when a later GC allocation fails?

ncsi_rsp_handler_gc() stores the allocation result before it updates the
counts:

	nc->mac_filter.addrs = kzalloc(size, GFP_ATOMIC);
	if (!nc->mac_filter.addrs)
		return -ENOMEM;
	nc->mac_filter.n_uc = rsp->uc_cnt;
	...
	nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
				       sizeof(*nc->vlan_filter.vids),
				       GFP_ATOMIC);
	if (!nc->vlan_filter.vids)
		return -ENOMEM;

Suppose a first GC succeeds and a second GC for the same channel hits a
GFP_ATOMIC failure. Then addrs or vids is NULL, but the old non-zero counts
are still there.

A second GC can come from NCSI_CMD_SEND_CMD, because ncsi_send_cmd_nl()
sets nca.type = hdr->type. It can also come from a device that answers any
pending request with a GC-type response, because ncsi_rcv_rsp() chooses the
handler from the response type.

The next GP response passes the new bound and runs either:

	memcpy(&ncmf->addrs[i * ETH_ALEN], pdata, ETH_ALEN);

or:

	ncvf->vids[i] = ntohs(*(__be16 *)pdata);

through a NULL base in softirq context. clear_one_vid() and set_one_vid()
would dereference the NULL vids pointer in the same way.

Could ncsi_rsp_handler_gc() allocate into local variables and install the
new pointers and counts together only on success? Resetting the counts to
zero on failure would also work.

[Severity: Medium]
Does nc->lock keep the count and the buffer consistent here?

The new bound reads n_uc + n_mc + n_mixed (and n_vids in the VLAN loop)
under nc->lock. The only writer, ncsi_rsp_handler_gc(), does not take
nc->lock. It also updates in two steps: it installs the new buffer first
and writes the counts afterwards.

ncsi_rcv_rsp() releases ndp->lock before it calls nrh->handler(). So
nothing in NCSI stops the GC and GP handlers from running at the same time.

Suppose GC and GP responses for the same channel are handled at the same
time on different CPUs. Can GP read the old, larger count together with the
new, smaller addrs buffer? That would give the same heap out-of-bounds
memcpy() this patch is trying to prevent.

In practice NCSI frames probably arrive on one RX queue, which would
serialize them, but nothing guarantees that. Would it help for
ncsi_rsp_handler_gc() to publish the pointers and counts together under
nc->lock?

>  		if (!(enable & (0x1 << i)))

[Severity: Medium]
This isn't a bug introduced by this patch, but the new limit of
sizeof(ncmf->bitmap) * 8 is wider than the enable fields.

Here enable holds rsp->mac_enable, which is 8 bits. In the VLAN loop it
holds ntohs(rsp->vlan_enable), which is 16 bits. The test is an int shift:

	if (!(enable & (0x1 << i)))

For MAC i >= 8 and VLAN i >= 16 there is no enable bit on the wire, so
those entries are always cleared.

Indexes 32 to 63 can still be reached when both the GC and GP counts are
above 32. For those, 0x1 << i is undefined behaviour (UBSAN
shift-out-of-bounds). On x86 the shift count wraps, so bits can be set
wrongly.

The same shift ran for i up to 254 before this patch. Would a limit based
on the enable field widths (8 for MAC, 16 for VLAN) avoid it?

>  			clear_bit(i, bitmap);
>  		else
> @@ -898,7 +900,8 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  	ncvf = &nc->vlan_filter;
>  	bitmap = &ncvf->bitmap;
>  	spin_lock_irqsave(&nc->lock, flags);
> -	for (i = 0; i < rsp->vlan_cnt; i++, pdata += 2) {
> +	for (i = 0; i < rsp->vlan_cnt && i < ncvf->n_vids &&
> +	     i < sizeof(ncvf->bitmap) * 8; i++, pdata += 2) {

[Severity: High]
The commit message says:

  "so any count larger than 64 writes out-of-bounds bits into struct
  ncsi_channel"

and:

  "mirroring the validation the SVF and SMA handlers already apply to their
  indices"

Does this patch fully close that problem?

The 64-bit limit is only added to the two GP loops. The root cause seems to
be ncsi_rsp_handler_gc(), which stores the device-supplied counts without
limiting them to the u64 bitmap:

	nc->vlan_filter.bitmap = U64_MAX;
	nc->vlan_filter.n_vids = rsp->vlan_cnt;

vlan_cnt can be up to 255, and uc_cnt + mc_cnt + mixed_cnt can be up to
765.

On every channel configuration, the ncsi_dev_state_config_clear_vids state
calls clear_one_vid() in net/ncsi/ncsi-manage.c repeatedly until it returns
-1:

	index = find_first_bit(bitmap, ncf->n_vids);
	...
	vid = ncf->vids[index];

	clear_bit(index, bitmap);
	ncf->vids[index] = 0;

With n_vids > 64, once the 64 real bits are cleared, find_first_bit() keeps
scanning into the next member, the u16 *vids pointer. Can clear_bit() then
clear bits in that kernel pointer, and the next line write through the
corrupted pointer? The only trigger needed is a GC response with
vlan_cnt > 64.

The same mismatch between the count and the bitmap width seems to exist in
these places:

  set_one_vid(): find_first_zero_bit() and set_bit() are bounded only by
  n_vids

  ncsi_rsp_handler_svf(): cmd->index is checked only against ncf->n_vids
  before set_bit()/test_and_clear_bit(cmd->index - 1, bitmap)

  ncsi_rsp_handler_sma(): cmd->index is checked only against
  n_uc + n_mc + n_mixed before set_bit()/clear_bit(cmd->index - 1, bitmap)

  ncsi_write_channel_info(): find_next_bit(&ncf->bitmap, ncf->n_vids, ...)
  reads past the u64

With NCSI_CMD_SEND_CMD, userspace chooses the SVF/SMA index, so the checks
the commit message mirrors have the same gap.

Would it be better to clamp or reject the counts in ncsi_rsp_handler_gc(),
so that n_vids is at most 64 and the MAC total is at most 64? Then the
sizeof(bitmap) * 8 limits in the GP loops would not be needed.

[Severity: High]
This isn't a bug introduced by this patch, but these loops can still read
past the end of the received GP response.

GP is registered as a variable-length response:

	{ NCSI_PKT_RSP_GP,     -1, ncsi_rsp_handler_gp      },

so ncsi_rcv_rsp() does:

	payload = nrh->payload;
	if (payload < 0)
		payload = ntohs(hdr->length);
	ret = ncsi_validate_rsp_pkt(nr, payload);

As a result, the ntohs(h->common.length) != payload check in
ncsi_validate_rsp_pkt() can never fail. The checksum is then read at an
offset the device controls, with no check against skb->len:

	pchecksum = (__be32 *)((void *)(h + 1) + ALIGN(payload, 4) - 4);

That offset can be up to about 64KB past the header.

Nothing in net/ncsi calls pskb_may_pull() or checks skb->len for
responses. So ncsi_rsp_handler_gp() reads 48 bytes plus 6 per MAC entry and
2 per VLAN entry from skb_network_header(). Only the device-supplied counts
limit how far it reads.

A short GP frame with large counts would copy bytes from beyond the frame
into ncmf->addrs and ncvf->vids[]. VIDs whose enable bits are set are later
reported by ncsi_write_channel_info() through NCSI_CMD_PKG_INFO, which
needs no privilege.

Should the response length be checked against the skb before the handler
walks these tables?

[Severity: Medium]
This is a pre-existing issue, but ncsi_rsp_handler_gc() leaks the previous
nc->mac_filter.addrs and nc->vlan_filter.vids buffers when a channel gets
more than one GC response:

	nc->mac_filter.addrs = kzalloc(size, GFP_ATOMIC);
	...
	nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
	...

Neither old buffer is freed before it is overwritten. The only kfree() of
these pointers is in ncsi_remove_channel().

A repeated GC can come from:

  NCSI_CMD_SEND_CMD

  a device sending a GC-type response to any pending request

  a re-probe, where ncsi_add_channel() returns the existing channel

Should the old buffers be freed when they are replaced?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/CAANnmVAicVNhy8fMUaFJ_N%2Bb4DnbwpYi1MOL4yMVisix1WCX4w%40mail.gmail.com