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

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

[Vamsi Krishna Kattamuri]

ncsi_rsp_handler_gp() iterates over the MAC address and VLAN filter
tables using the counts from the Get Parameters response itself,
rsp->mac_cnt and rsp->vlan_cnt.  The arrays it writes into, however,
were allocated by the Get Controller Capabilities handler from the
independent rsp->uc_cnt, rsp->mc_cnt, rsp->mixed_cnt and rsp->vlan_cnt
fields.

A malicious or buggy NCSI device can therefore report contradictory
counts in the two responses - for example a GC response advertising
a single unicast address filter followed by a GP response with
mac_cnt = 255.  The GP handler then memcpy()s 255 MAC addresses into
the 6-byte kzalloc()ed buffer, a heap out-of-bounds write of roughly
1.5KB of device-controlled data, and the same applies to vlan_cnt
versus the vids[] array.

The loop indices are additionally used as bit numbers for
set_bit()/clear_bit() on the u64 filter bitmaps, so any count larger
than 64 writes out-of-bounds bits into struct ncsi_channel.

Clamp both loops to the number of entries actually allocated for the
channel and to the capacity of the bitmap word, mirroring the
validation the SVF and SMA handlers already apply to their indices.

Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler")
Cc: stable@vger.kernel.org
Signed-off-by: Vamsi Krishna Kattamuri <kattamurivamsikrishna17@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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) {
 		if (!(enable & (0x1 << i)))
 			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) {
 		if (!(enable & (0x1 << i)))
 			clear_bit(i, bitmap);
 		else
-- 
2.47.3

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 (1) · Medium: 1 (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.
- [Medium] The new MAC loop clamp in ncsi_rsp_handler_gp()
  (net/ncsi/ncsi-rsp.c:886-888) also stops `pdata` from advancing.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the NCSI
  response path never checks packet contents against the actual skb…
- [Medium] This problem wasn't introduced by this patch, but the new
  64-entry clamp in ncsi_rsp_handler_gp() still allows i in 32..63,…
- [Medium] This problem wasn't introduced by this patch, but
  ncsi_rsp_handler_gc() publishes new addrs/vids pointers and their…

--

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

The patch limits the MAC and VLAN filter loops in ncsi_rsp_handler_gp().
Each loop now stops at the number of entries allocated by the Get
Controller Capabilities handler, and at the 64-bit width of the filter
bitmap. The goal is to stop heap and bitmap out-of-bounds writes when the
device reports contradictory counts in the GC and GP responses.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a3..fa1ed405dfbb7 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]
Does this clamp also change where the VLAN table is read from?

In the GP response, the VLAN tag table starts at offset 48 + mac_cnt * 6.
ncsi_rsp_handler_gp() only gets there through the pdata += 6 side effect
of this loop. When rsp->mac_cnt is larger than
min(n_uc + n_mc + n_mixed, 64), the loop now exits early and pdata is
left inside the MAC table.

Take the scenario from the commit message: GC advertises one filter and
GP has mac_cnt = 255. The VLAN loop below would then decode MAC address
bytes as VLAN IDs:

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

It would also apply the vlan_enable bits to those entries. Later,
set_one_vid() compares ndp->vlan_vids against these vids[] entries, and
NCSI_CMD_PKG_INFO reports them to userspace.

Could the packet offset be kept separate from the storage clamp? For
example, the VLAN start could be computed as
(unsigned char *)rsp + 48 + rsp->mac_cnt * ETH_ALEN, after checking it
against the payload length.

[Severity: Medium]
This is a pre-existing issue, but ncsi_rsp_handler_gc() publishes the new
addrs and vids pointers and their counts without taking nc->lock. It also
stores each pointer before its 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->mac_filter.n_mc = rsp->mc_cnt;
	nc->mac_filter.n_mixed = rsp->mixed_cnt;

The new clamp reads these counts under nc->lock. However, ncsi_rcv_rsp()
calls nrh->handler(nr) after dropping ndp->lock, so the NCSI core does
not serialize response handlers.

Suppose a GC and a GP response for the same channel are processed at the
same time on different CPUs. Could GP pair the new, smaller buffer with
the old, larger counts and still write out of bounds despite the new
clamp? This would need NCSI frames to be delivered on multiple RX queues,
which may not happen on typical single-queue BMC MACs.

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

[Severity: Medium]
This isn't a bug introduced by this patch, but with the new 64-entry
bound i can still be anywhere from 32 to 63 here. In that case 0x1 << i
shifts an int by 32 or more. Isn't that undefined behaviour, which UBSAN
reports as shift-out-of-bounds?

The packet only carries 8 MAC enable bits (unsigned char mac_enable) and
16 VLAN enable bits (__be16 vlan_enable). On x86 the shift count wraps.
For i = 32..39, the enable bits of entries 0..7 are reused, so set_bit()
marks filters that the device never enabled. The VLAN loop uses the same
expression and has the same problem for i = 32..47.

Before this patch the range was larger (i could reach 254). The patch
narrows the problem but does not remove 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]
Does this fully close the out-of-bounds bitmap writes that the commit
message describes? The commit message says:

  The loop indices are additionally used as bit numbers for
  set_bit()/clear_bit() on the u64 filter bitmaps, so any count larger
  than 64 writes out-of-bounds bits into struct ncsi_channel.

The counts come from ncsi_rsp_handler_gc(), which does not clamp them:

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

vlan_cnt can be as large as 255, and n_uc + n_mc + n_mixed as large as
765. Other users still treat these counts as the bitmap size.

clear_one_vid() runs during the config clear_vids state:

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

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

Once bits 0..63 are cleared, find_first_bit() continues into the next
field of struct ncsi_channel_vlan_filter, which is the vids pointer.
clear_bit() then clears a bit of that pointer, and ncf->vids[index] = 0
writes through the modified pointer. Can a GC response with
vlan_cnt > 64 trigger this on its own, without any GP response?

set_one_vid() follows the same pattern, using find_first_zero_bit() and
set_bit() sized by n_vids. ncsi_write_channel_info() calls
find_next_bit() over n_vids bits of the single u64.

The commit message also says the clamp mirrors "the validation the SVF
and SMA handlers already apply to their indices". Is that accurate?
Those handlers only check the index against the table size.

ncsi_rsp_handler_svf():

	if (cmd->index == 0 || cmd->index > ncf->n_vids)
		return -ERANGE;

ncsi_rsp_handler_sma():

	if (cmd->index == 0 ||
	    cmd->index > ncf->n_uc + ncf->n_mc + ncf->n_mixed)
		return -ERANGE;

Both then call set_bit(cmd->index - 1, bitmap). In the SMA case, that can
flip bits in the adjacent addrs pointer.

Would it be better to cap the counts at 64, or reject the response, in
ncsi_rsp_handler_gc()? That one change would cover GP, SVF, SMA,
set_one_vid(), clear_one_vid() and the netlink path. As it stands, a
stable backport of this patch alone would leave those paths reachable.

[Severity: High]
This is a pre-existing issue, but is pdata ever checked against the
length of the received frame here?

GP is registered as variable length:

	{ NCSI_PKT_RSP_GP,     -1, ncsi_rsp_handler_gp      },

So ncsi_rcv_rsp() takes the payload length from the device's own header:

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

ncsi_validate_rsp_pkt() then reads the checksum at an offset derived from
that payload, without checking skb->len, and it accepts a zero checksum.
There doesn't seem to be any pskb_may_pull() or skb->len check on the
NCSI RX path.

ncsi_rsp_handler_gp() then walks pdata for up to
48 + 64 * 6 + 64 * 2 bytes, based only on mac_cnt and vlan_cnt. With a
short GP frame and large counts, could this copy bytes from past the end
of the received frame into mac_filter.addrs and vlan_filter.vids?

Those vids[] entries are later sent to userspace by
ncsi_write_channel_info() through NCSI_CMD_PKG_INFO. That command is not
marked GENL_ADMIN_PERM.

>  		if (!(enable & (0x1 << i)))
>  			clear_bit(i, bitmap);
>  		else

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

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

[Vamsi Krishna Kattamuri]

ncsi_rsp_handler_gp() iterates over the MAC address and VLAN filter
tables using the counts from the Get Parameters response itself,
rsp->mac_cnt and rsp->vlan_cnt.  The arrays it writes into, however,
were allocated by the Get Controller Capabilities handler from the
independent rsp->uc_cnt, rsp->mc_cnt, rsp->mixed_cnt and rsp->vlan_cnt
fields.

A malicious or buggy NCSI device can therefore report contradictory
counts in the two responses - for example a GC response advertising
a single unicast address filter followed by a GP response with
mac_cnt = 255.  The GP handler then memcpy()s 255 MAC addresses into
the 6-byte kzalloc()ed buffer, a heap out-of-bounds write of roughly
1.5KB of device-controlled data, and the same applies to vlan_cnt
versus the vids[] array.

The loop indices are additionally used as bit numbers for
set_bit()/clear_bit() on the u64 filter bitmaps, so any count larger
than 64 writes out-of-bounds bits into struct ncsi_channel.

Clamp both loops to the number of entries actually allocated for the
channel and to the capacity of the bitmap word, mirroring the
validation the SVF and SMA handlers already apply to their indices.

Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler")
Cc: stable@vger.kernel.org
Signed-off-by: Vamsi Krishna Kattamuri <kattamurivamsikrishna17@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

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) {
 		if (!(enable & (0x1 << i)))
 			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) {
 		if (!(enable & (0x1 << i)))
 			clear_bit(i, bitmap);
 		else
-- 
2.47.3

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

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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