[PATCH net 1/6] net/ncsi: validate response packet lengths against the skb

[PATCH net 1/6] net/ncsi: validate response packet lengths against the skb

[Michael Bommarito]

ncsi_rcv_rsp() reads the common packet header before checking that the
skb contains enough data for it, and ncsi_validate_rsp_pkt() trusts
the response payload length before accessing the checksum field.

Malformed NC-SI replies can therefore drive header and checksum reads
past the received packet body. Make the dispatcher pull the common
header first, then have ncsi_validate_rsp_pkt() pull the full response
body before validating the packet.

This keeps malformed responses on the error path instead of letting the
parser walk past the skb payload.

Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index fbd84bc8026a..1fe061ede26d 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -38,11 +38,18 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 	struct ncsi_rsp_pkt_hdr *h;
 	u32 checksum;
 	__be32 *pchecksum;
+	unsigned int len;
 
 	/* Check NCSI packet header. We don't need validate
 	 * the packet type, which should have been checked
 	 * before calling this function.
 	 */
+	len = skb_network_offset(nr->rsp) + sizeof(*h) + ALIGN(payload, 4);
+	if (!pskb_may_pull(nr->rsp, len)) {
+		netdev_dbg(nr->ndp->ndev.dev, "NCSI: packet too short\n");
+		return -EINVAL;
+	}
+
 	h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(nr->rsp);
 
 	if (h->common.revision != NCSI_PKT_REVISION) {
@@ -1182,6 +1189,11 @@ int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
 	}
 
 	/* Check if it is AEN packet */
+	if (!pskb_may_pull(skb, skb_network_offset(skb) + sizeof(*hdr))) {
+		ret = -EINVAL;
+		goto err_free_skb;
+	}
+
 	hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
 	if (hdr->type == NCSI_PKT_AEN)
 		return ncsi_aen_handler(ndp, skb);
-- 
2.53.0

[PATCH net 2/6] net/ncsi: bound filter table state to software limits

[Michael Bommarito]

The NCSI filter state uses single-word bitmaps for both MAC and VLAN
entries, but Get Capabilities and Get Parameters responses can still
feed larger counts into that state.

Cap the stored VLAN table size to the bitmap width before it reaches
the manage-side bitmap walkers, reject GP tables that exceed the sizes
advertised by GC, and stop indexing the MAC filter bitmap past its
software capacity. Also stop shifting past the width of the enable
bitfields when GP reports more entries than fit in those masks.

This keeps oversized or inconsistent filter counts from turning into
out-of-bounds bitmap accesses and oversized table walks in the response
and manage paths. A follow-up patch in this series separately validates
that the GP payload actually covers the consumed MAC/VLAN table bytes.

A live x86_64/KASAN QEMU repro can drive this after GC advertises a
single MAC filter slot and GP then reports mac_cnt=65. Without this
change, KASAN reports a slab-out-of-bounds write in
ncsi_rsp_handler_gp(); with this change applied, the same reply is
rejected with -ERANGE.

Fixes: 062b3e1b6d4f ("net/ncsi: Refactor MAC, VLAN filters")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 46 ++++++++++++++++++++++++++++++++++++---------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 1fe061ede26d..47ddf2bbb13b 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -22,6 +22,8 @@
 /* Nibbles within [0xA, 0xF] add zero "0" to the returned value.
  * Optional fields (encoded as 0xFF) will default to zero.
  */
+#define NCSI_FILTER_BITS	BITS_PER_TYPE(u64)
+
 static u8 decode_bcd_u8(u8 x)
 {
 	int lo = x & 0xF;
@@ -32,6 +34,12 @@ static u8 decode_bcd_u8(u8 x)
 	return lo + hi * 10;
 }
 
+static bool ncsi_filter_is_enabled(unsigned long enable, unsigned int index,
+				      unsigned int nbits)
+{
+	return index < nbits && (enable & BIT(index));
+}
+
 static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 				 unsigned short payload)
 {
@@ -481,7 +489,8 @@ static int ncsi_rsp_handler_sma(struct ncsi_request *nr)
 	bitmap = &ncf->bitmap;
 
 	if (cmd->index == 0 ||
-	    cmd->index > ncf->n_uc + ncf->n_mc + ncf->n_mixed)
+	    cmd->index > ncf->n_uc + ncf->n_mc + ncf->n_mixed ||
+	    cmd->index > NCSI_FILTER_BITS)
 		return -ERANGE;
 
 	index = (cmd->index - 1) * ETH_ALEN;
@@ -798,6 +807,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
 	struct ncsi_channel *nc;
 	struct ncsi_package *np;
 	size_t size;
+	unsigned int vlan_cnt;
 
 	/* Find the channel */
 	rsp = (struct ncsi_rsp_gc_pkt *)skb_network_header(nr->rsp);
@@ -819,6 +829,12 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
 	nc->caps[NCSI_CAP_VLAN].cap = rsp->vlan_mode &
 				      NCSI_CAP_VLAN_MASK;
 
+	vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, NCSI_FILTER_BITS);
+	if (vlan_cnt != rsp->vlan_cnt)
+		netdev_warn(ndp->ndev.dev,
+			    "NCSI: VLAN filter count %u exceeds software limit %u\n",
+			    rsp->vlan_cnt, (unsigned int)NCSI_FILTER_BITS);
+
 	size = (rsp->uc_cnt + rsp->mc_cnt + rsp->mixed_cnt) * ETH_ALEN;
 	nc->mac_filter.addrs = kzalloc(size, GFP_ATOMIC);
 	if (!nc->mac_filter.addrs)
@@ -827,7 +843,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
 	nc->mac_filter.n_mc = rsp->mc_cnt;
 	nc->mac_filter.n_mixed = rsp->mixed_cnt;
 
-	nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
+	nc->vlan_filter.vids = kcalloc(vlan_cnt,
 				       sizeof(*nc->vlan_filter.vids),
 				       GFP_ATOMIC);
 	if (!nc->vlan_filter.vids)
@@ -836,7 +852,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
 	 * configuration state
 	 */
 	nc->vlan_filter.bitmap = U64_MAX;
-	nc->vlan_filter.n_vids = rsp->vlan_cnt;
+	nc->vlan_filter.n_vids = vlan_cnt;
 	np->ndp->channel_count = rsp->channel_cnt;
 
 	return 0;
@@ -853,6 +869,9 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	unsigned char *pdata;
 	unsigned long flags;
 	void *bitmap;
+	unsigned int mac_cnt;
+	unsigned int mac_nbits;
+	unsigned int vlan_cnt;
 	int i;
 
 	/* Find the channel */
@@ -862,6 +881,15 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	if (!nc)
 		return -ENODEV;
 
+	ncmf = &nc->mac_filter;
+	ncvf = &nc->vlan_filter;
+	mac_cnt = min_t(unsigned int, rsp->mac_cnt, NCSI_FILTER_BITS);
+	mac_nbits = ncmf->n_uc + ncmf->n_mc + ncmf->n_mixed;
+	vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, ncvf->n_vids);
+
+	if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
+		return -ERANGE;
+
 	/* Modes with explicit enabled indications */
 	if (ntohl(rsp->valid_modes) & 0x1) {	/* BC filter mode */
 		nc->modes[NCSI_MODE_BC].enable = 1;
@@ -887,11 +915,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	/* MAC addresses filter table */
 	pdata = (unsigned char *)rsp + 48;
 	enable = rsp->mac_enable;
-	ncmf = &nc->mac_filter;
 	spin_lock_irqsave(&nc->lock, flags);
 	bitmap = &ncmf->bitmap;
-	for (i = 0; i < rsp->mac_cnt; i++, pdata += 6) {
-		if (!(enable & (0x1 << i)))
+	for (i = 0; i < mac_cnt; i++, pdata += 6) {
+		if (!ncsi_filter_is_enabled(enable, i,
+					    BITS_PER_TYPE(rsp->mac_enable)))
 			clear_bit(i, bitmap);
 		else
 			set_bit(i, bitmap);
@@ -902,11 +930,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 
 	/* VLAN filter table */
 	enable = ntohs(rsp->vlan_enable);
-	ncvf = &nc->vlan_filter;
 	bitmap = &ncvf->bitmap;
 	spin_lock_irqsave(&nc->lock, flags);
-	for (i = 0; i < rsp->vlan_cnt; i++, pdata += 2) {
-		if (!(enable & (0x1 << i)))
+	for (i = 0; i < vlan_cnt; i++, pdata += 2) {
+		if (!ncsi_filter_is_enabled(enable, i,
+					    BITS_PER_TYPE(rsp->vlan_enable)))
 			clear_bit(i, bitmap);
 		else
 			set_bit(i, bitmap);
-- 
2.53.0

[PATCH net 3/6] net/ncsi: validate GMCMA address counts against the payload

[Michael Bommarito]

Get MC MAC Address responses carry a flexible array of provisioned
addresses, but the handler currently trusts address_count without first
checking that the advertised payload actually contains that many MAC
entries.

Validate the fixed GMCMA fields plus checksum, then make sure the
address_count fits in the remaining payload before the handler walks
the address array.

Fixes: b8291cf3d118 ("net/ncsi: Add NC-SI 1.2 Get MC MAC Address command")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 47ddf2bbb13b..cbddb2012f90 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -40,6 +40,14 @@ static bool ncsi_filter_is_enabled(unsigned long enable, unsigned int index,
 	return index < nbits && (enable & BIT(index));
 }
 
+static unsigned int ncsi_rsp_payload(struct sk_buff *skb)
+{
+	struct ncsi_rsp_pkt_hdr *h;
+
+	h = (struct ncsi_rsp_pkt_hdr *)skb_network_header(skb);
+	return ntohs(h->common.length);
+}
+
 static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 				 unsigned short payload)
 {
@@ -1127,9 +1135,21 @@ static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
 	struct sockaddr_storage *saddr = &ndp->pending_mac;
 	struct net_device *ndev = ndp->ndev.dev;
 	struct ncsi_rsp_gmcma_pkt *rsp;
+	unsigned int addr_bytes;
+	unsigned int payload;
 	int i;
 
 	rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->address_count) + sizeof(rsp->reserved) +
+		      sizeof(__be32))
+		return -EINVAL;
+
+	addr_bytes = payload - sizeof(rsp->address_count) -
+		     sizeof(rsp->reserved) - sizeof(__be32);
+	if (rsp->address_count > addr_bytes / ETH_ALEN)
+		return -EINVAL;
+
 	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
 
 	netdev_info(ndev, "NCSI: Received %d provisioned MAC addresses\n",
-- 
2.53.0

[PATCH net 4/6] net/ncsi: validate OEM response payloads before parsing

[Michael Bommarito]

Reject truncated OEM responses before reading the manufacturer ID,
vendor-specific subheaders, or vendor MAC address payloads.

The OEM response dispatcher reads rsp->mfr_id without verifying that the
skb contains the manufacturer field and checksum. The Mellanox,
Broadcom, and Intel handlers then read their command-specific headers
without checking that the payload is large enough for those fields. The
shared GMA helper finally copies a MAC address from a
manufacturer-specific offset without validating that the payload reaches
that offset.

Validate the advertised payload before each of those reads so malformed
or truncated BMC responses are rejected before the parser touches data
past the end of the skb.

Fixes: fb4ee67529ff ("net/ncsi: Add NCSI OEM command support")
Fixes: cb10c7c0dfd9 ("net/ncsi: Add NCSI Broadcom OEM command")
Fixes: 16e8c4ca21a2 ("net/ncsi: Add NCSI Mellanox OEM command")
Fixes: 205b95fe658d ("net/ncsi: add get MAC address command to get Intel i210 MAC address")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index cbddb2012f90..94354dca23ea 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -656,6 +656,7 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	struct net_device *ndev = ndp->ndev.dev;
 	struct ncsi_rsp_oem_pkt *rsp;
 	u32 mac_addr_off = 0;
+	unsigned int payload;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
@@ -668,6 +669,11 @@ static int ncsi_rsp_handler_oem_gma(struct ncsi_request *nr, int mfr_id)
 	else if (mfr_id == NCSI_OEM_MFR_INTEL_ID)
 		mac_addr_off = INTEL_MAC_ADDR_OFFSET;
 
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->mfr_id) + mac_addr_off + ETH_ALEN +
+		      sizeof(__be32))
+		return -EINVAL;
+
 	saddr->ss_family = ndev->type;
 	memcpy(saddr->__data, &rsp->data[mac_addr_off], ETH_ALEN);
 	if (mfr_id == NCSI_OEM_MFR_BCM_ID || mfr_id == NCSI_OEM_MFR_INTEL_ID)
@@ -686,9 +692,14 @@ static int ncsi_rsp_handler_oem_mlx(struct ncsi_request *nr)
 {
 	struct ncsi_rsp_oem_mlx_pkt *mlx;
 	struct ncsi_rsp_oem_pkt *rsp;
+	unsigned int payload;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->mfr_id) + sizeof(*mlx) + sizeof(__be32))
+		return -EINVAL;
+
 	mlx = (struct ncsi_rsp_oem_mlx_pkt *)(rsp->data);
 
 	if (mlx->cmd == NCSI_OEM_MLX_CMD_GMA &&
@@ -702,9 +713,14 @@ static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 {
 	struct ncsi_rsp_oem_bcm_pkt *bcm;
 	struct ncsi_rsp_oem_pkt *rsp;
+	unsigned int payload;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->mfr_id) + sizeof(*bcm) + sizeof(__be32))
+		return -EINVAL;
+
 	bcm = (struct ncsi_rsp_oem_bcm_pkt *)(rsp->data);
 
 	if (bcm->type == NCSI_OEM_BCM_CMD_GMA)
@@ -717,9 +733,14 @@ static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
 {
 	struct ncsi_rsp_oem_intel_pkt *intel;
 	struct ncsi_rsp_oem_pkt *rsp;
+	unsigned int payload;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->mfr_id) + sizeof(*intel) + sizeof(__be32))
+		return -EINVAL;
+
 	intel = (struct ncsi_rsp_oem_intel_pkt *)(rsp->data);
 
 	if (intel->cmd == NCSI_OEM_INTEL_CMD_GMA)
@@ -742,10 +763,15 @@ static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
 {
 	struct ncsi_rsp_oem_handler *nrh = NULL;
 	struct ncsi_rsp_oem_pkt *rsp;
+	unsigned int payload;
 	unsigned int mfr_id, i;
 
 	/* Get the response header */
 	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+	payload = ncsi_rsp_payload(nr->rsp);
+	if (payload < sizeof(rsp->mfr_id) + sizeof(__be32))
+		return -EINVAL;
+
 	mfr_id = ntohl(rsp->mfr_id);
 
 	/* Check for manufacturer id and Find the handler */
-- 
2.53.0

[PATCH net 5/6] net/ncsi: validate AEN packet lengths against the skb

[Michael Bommarito]

AEN packets are dispatched after only pulling the 16-byte common
header. ncsi_aen_handler() then reads the 20-byte AEN header to
select a per-type handler, and ncsi_validate_aen_pkt() walks
farther into the payload and checksum without first ensuring the
skb contains those bytes.

Pull the AEN-specific header before reading h->type, and pull the
full AEN header plus aligned payload before checksum validation.
That keeps short AEN packets from reading past the skb tail on the
AEN path.

Fixes: 2d283bdd079c ("net/ncsi: Resource management")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-aen.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/net/ncsi/ncsi-aen.c b/net/ncsi/ncsi-aen.c
index 040a31557201..cd34ef144cf8 100644
--- a/net/ncsi/ncsi-aen.c
+++ b/net/ncsi/ncsi-aen.c
@@ -16,11 +16,19 @@
 #include "internal.h"
 #include "ncsi-pkt.h"
 
-static int ncsi_validate_aen_pkt(struct ncsi_aen_pkt_hdr *h,
+static int ncsi_validate_aen_pkt(struct sk_buff *skb,
 				 const unsigned short payload)
 {
+	struct ncsi_aen_pkt_hdr *h;
 	u32 checksum;
 	__be32 *pchecksum;
+	unsigned int len;
+
+	len = skb_network_offset(skb) + sizeof(*h) + ALIGN(payload, 4);
+	if (!pskb_may_pull(skb, len))
+		return -EINVAL;
+
+	h = (struct ncsi_aen_pkt_hdr *)skb_network_header(skb);
 
 	if (h->common.revision != NCSI_PKT_REVISION)
 		return -EINVAL;
@@ -31,7 +39,7 @@ static int ncsi_validate_aen_pkt(struct ncsi_aen_pkt_hdr *h,
 	 * sender doesn't support checksum according to NCSI
 	 * specification.
 	 */
-	pchecksum = (__be32 *)((void *)(h + 1) + payload - 4);
+	pchecksum = (__be32 *)((void *)(h + 1) + ALIGN(payload, 4) - 4);
 	if (ntohl(*pchecksum) == 0)
 		return 0;
 
@@ -210,12 +218,19 @@ int ncsi_aen_handler(struct ncsi_dev_priv *ndp, struct sk_buff *skb)
 {
 	struct ncsi_aen_pkt_hdr *h;
 	struct ncsi_aen_handler *nah = NULL;
+	unsigned char type;
 	int i, ret;
 
+	if (!pskb_may_pull(skb, skb_network_offset(skb) + sizeof(*h))) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	/* Find the handler */
 	h = (struct ncsi_aen_pkt_hdr *)skb_network_header(skb);
+	type = h->type;
 	for (i = 0; i < ARRAY_SIZE(ncsi_aen_handlers); i++) {
-		if (ncsi_aen_handlers[i].type == h->type) {
+		if (ncsi_aen_handlers[i].type == type) {
 			nah = &ncsi_aen_handlers[i];
 			break;
 		}
@@ -223,24 +238,25 @@ int ncsi_aen_handler(struct ncsi_dev_priv *ndp, struct sk_buff *skb)
 
 	if (!nah) {
 		netdev_warn(ndp->ndev.dev, "Invalid AEN (0x%x) received\n",
-			    h->type);
+			    type);
 		ret = -ENOENT;
 		goto out;
 	}
 
-	ret = ncsi_validate_aen_pkt(h, nah->payload);
+	ret = ncsi_validate_aen_pkt(skb, nah->payload);
 	if (ret) {
 		netdev_warn(ndp->ndev.dev,
 			    "NCSI: 'bad' packet ignored for AEN type 0x%x\n",
-			    h->type);
+			    type);
 		goto out;
 	}
 
+	h = (struct ncsi_aen_pkt_hdr *)skb_network_header(skb);
 	ret = nah->handler(ndp, h);
 	if (ret)
 		netdev_err(ndp->ndev.dev,
 			   "NCSI: Handler for AEN type 0x%x returned %d\n",
-			   h->type, ret);
+			   type, ret);
 out:
 	consume_skb(skb);
 	return ret;
-- 
2.53.0

[PATCH net 6/6] net/ncsi: validate GP payload lengths before parsing

[Michael Bommarito]

ncsi_rsp_handler_gp() now bounds MAC and VLAN counts to software
and GC-reported limits, but it still assumes the advertised GP
payload is large enough for the fixed fields plus the consumed
filter-table bytes. A short GP reply can still make parsing start
past the payload or walk beyond its tail.

Validate that the declared GP payload covers the fixed GP prefix,
the consumed MAC and VLAN entries, and the checksum before parsing
the filter tables.

Fixes: 062b3e1b6d4f ("net/ncsi: Refactor MAC, VLAN filters")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-7
Signed-off-by: Michael Bommarito <michael.bommarito@gmail.com>
---
 net/ncsi/ncsi-rsp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 94354dca23ea..565d38fd4b92 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -899,6 +899,8 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	struct ncsi_dev_priv *ndp = nr->ndp;
 	struct ncsi_rsp_gp_pkt *rsp;
 	struct ncsi_channel *nc;
+	size_t needed;
+	unsigned int payload;
 	unsigned short enable;
 	unsigned char *pdata;
 	unsigned long flags;
@@ -924,6 +926,14 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
 		return -ERANGE;
 
+	payload = ncsi_rsp_payload(nr->rsp);
+	needed = offsetof(struct ncsi_rsp_gp_pkt, mac) - sizeof(rsp->rsp);
+	needed += mac_cnt * ETH_ALEN;
+	needed += vlan_cnt * sizeof(__be16);
+	needed += sizeof(rsp->checksum);
+	if (payload < needed)
+		return -EINVAL;
+
 	/* Modes with explicit enabled indications */
 	if (ntohl(rsp->valid_modes) & 0x1) {	/* BC filter mode */
 		nc->modes[NCSI_MODE_BC].enable = 1;
-- 
2.53.0

Re: [PATCH net 1/6] net/ncsi: validate response packet lengths against the skb

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/ncsi: validate response packet lengths against the skb

This commit adds packet length validation for NC-SI responses. It pulls the
common header first, then pulls the full response body before validating the
packet to prevent reading past the received packet body.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index fbd84bc8026a3..1fe061ede26dd 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -38,11 +38,18 @@ static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
>  	struct ncsi_rsp_pkt_hdr *h;
>  	u32 checksum;
>  	__be32 *pchecksum;
> +	unsigned int len;
>  
>  	/* Check NCSI packet header. We don't need validate
>  	 * the packet type, which should have been checked
>  	 * before calling this function.
>  	 */
> +	len = skb_network_offset(nr->rsp) + sizeof(*h) + ALIGN(payload, 4);
> +	if (!pskb_may_pull(nr->rsp, len)) {

Does this pskb_may_pull() call potentially invalidate the hdr pointer cached
earlier in ncsi_rcv_rsp()?

Looking at ncsi_rcv_rsp(), it caches the header pointer before validating:

ncsi_rcv_rsp() {
    ...
    hdr = (struct ncsi_pkt_hdr *)skb_network_header(skb);
    ...
    ret = ncsi_validate_rsp_pkt(nr, payload);

If the SKB is fragmented and pskb_may_pull() needs to pull data into the
linear area, it can reallocate the SKB's data buffer and free the old buffer.

When ncsi_validate_rsp_pkt() returns, the local hdr pointer in ncsi_rcv_rsp()
would still point to the freed memory.

This pointer is then dereferenced in multiple error paths:

    if (ret) {
        netdev_warn(ndp->ndev.dev,
                    "NCSI: 'bad' packet ignored for type 0x%x\n",
                    hdr->type);
...
    ret = nrh->handler(nr);
    if (ret)
        netdev_err(ndp->ndev.dev,
                   "NCSI: Handler for packet type 0x%x returned %d\n",
                   hdr->type, ret);

Could this cause a use-after-free when reading hdr->type?
-- 
pw-bot: cr

Re: [PATCH net 2/6] net/ncsi: bound filter table state to software limits

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/ncsi: bound filter table state to software limits

The NCSI filter state uses single-word bitmaps for both MAC and VLAN
entries, but Get Capabilities and Get Parameters responses can still
feed larger counts into that state. This patch caps the stored table
sizes to the bitmap width to prevent out-of-bounds accesses.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 1fe061ede26dd..47ddf2bbb13bb 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
[ ... ]
> @@ -819,6 +829,12 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
>  	nc->caps[NCSI_CAP_VLAN].cap = rsp->vlan_mode &
>  				      NCSI_CAP_VLAN_MASK;
>  
> +	vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, NCSI_FILTER_BITS);
> +	if (vlan_cnt != rsp->vlan_cnt)
> +		netdev_warn(ndp->ndev.dev,
> +			    "NCSI: VLAN filter count %u exceeds software limit %u\n",
> +			    rsp->vlan_cnt, (unsigned int)NCSI_FILTER_BITS);
> +
>  	size = (rsp->uc_cnt + rsp->mc_cnt + rsp->mixed_cnt) * ETH_ALEN;
>  	nc->mac_filter.addrs = kzalloc(size, GFP_ATOMIC);
>  	if (!nc->mac_filter.addrs)
> @@ -827,7 +843,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
>  	nc->mac_filter.n_mc = rsp->mc_cnt;
>  	nc->mac_filter.n_mixed = rsp->mixed_cnt;
>  
> -	nc->vlan_filter.vids = kcalloc(rsp->vlan_cnt,
> +	nc->vlan_filter.vids = kcalloc(vlan_cnt,
>  				       sizeof(*nc->vlan_filter.vids),
>  				       GFP_ATOMIC);
>  	if (!nc->vlan_filter.vids)
> @@ -836,7 +852,7 @@ static int ncsi_rsp_handler_gc(struct ncsi_request *nr)
>  	 * configuration state
>  	 */
>  	nc->vlan_filter.bitmap = U64_MAX;
> -	nc->vlan_filter.n_vids = rsp->vlan_cnt;
> +	nc->vlan_filter.n_vids = vlan_cnt;

[ ... ]

> @@ -862,6 +881,15 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  	if (!nc)
>  		return -ENODEV;
>  
> +	ncmf = &nc->mac_filter;
> +	ncvf = &nc->vlan_filter;
> +	mac_cnt = min_t(unsigned int, rsp->mac_cnt, NCSI_FILTER_BITS);
> +	mac_nbits = ncmf->n_uc + ncmf->n_mc + ncmf->n_mixed;
> +	vlan_cnt = min_t(unsigned int, rsp->vlan_cnt, ncvf->n_vids);
> +
> +	if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
> +		return -ERANGE;

Because nc->vlan_filter.n_vids is capped to 64 in the GC response handler,
could this check entirely reject GP responses from compliant hardware that
supports and reports more than 64 VLAN filters?

>  	/* Modes with explicit enabled indications */
>  	if (ntohl(rsp->valid_modes) & 0x1) {	/* BC filter mode */
[ ... ]
> @@ -887,11 +915,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  	/* MAC addresses filter table */
>  	pdata = (unsigned char *)rsp + 48;
>  	enable = rsp->mac_enable;
> -	ncmf = &nc->mac_filter;
>  	spin_lock_irqsave(&nc->lock, flags);
>  	bitmap = &ncmf->bitmap;
> -	for (i = 0; i < rsp->mac_cnt; i++, pdata += 6) {
> -		if (!(enable & (0x1 << i)))
> +	for (i = 0; i < mac_cnt; i++, pdata += 6) {
> +		if (!ncsi_filter_is_enabled(enable, i,
> +					    BITS_PER_TYPE(rsp->mac_enable)))
>  			clear_bit(i, bitmap);
>  		else
>  			set_bit(i, bitmap);

If the device reports a MAC count greater than 64 but still within the
software limit of mac_nbits, this loop will terminate at 64 due to the
mac_cnt cap.

Will this cause pdata to be advanced by only 384 bytes instead of the
actual size of the MAC array in the packet?

> @@ -902,11 +930,11 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  
>  	/* VLAN filter table */
>  	enable = ntohs(rsp->vlan_enable);
> -	ncvf = &nc->vlan_filter;
>  	bitmap = &ncvf->bitmap;
>  	spin_lock_irqsave(&nc->lock, flags);
> -	for (i = 0; i < rsp->vlan_cnt; i++, pdata += 2) {
> -		if (!(enable & (0x1 << i)))
> +	for (i = 0; i < vlan_cnt; i++, pdata += 2) {
> +		if (!ncsi_filter_is_enabled(enable, i,
> +					    BITS_PER_TYPE(rsp->vlan_enable)))

If pdata is not advanced past all MAC addresses from the payload, does this
cause the VLAN filter loop to misinterpret the remaining MAC address bytes
as VLAN tags?


While reviewing this patch, I noticed a few pre-existing regressions in the
surrounding codebase that might be worth looking into.

In ncsi_unregister_dev(), the subsystem frees all packages and channels by
iterating and calling ncsi_remove_package(), and then calls
disable_work_sync(&ndp->work). If ncsi_dev_work is executing concurrently,
could it dereference ndp->active_package and ndp->active_channel after they
have been freed?

Also in ncsi_unregister_dev(), ndp is freed using kfree(), but it doesn't
appear to iterate over ndp->requests to cancel their associated timers via
del_timer_sync(). If a pending NCSI request times out after the device is
unregistered, could the timer callback dereference the freed ndp pointer?

Finally, ncsi_remove_package() and ncsi_remove_channel() unlink objects
using list_del_rcu(), but seem to immediately free the objects using kfree()
instead of kfree_rcu() or synchronize_rcu(). Could concurrent readers
traversing these lists under rcu_read_lock() access freed memory?

Re: [PATCH net 3/6] net/ncsi: validate GMCMA address counts against the payload

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/ncsi: validate GMCMA address counts against the payload

This commit adds payload length checks to the NC-SI GMCMA response handler
to ensure the advertised address count does not exceed the packet size.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c

[ ... ]

> @@ -1127,9 +1135,21 @@ static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
>  	struct sockaddr_storage *saddr = &ndp->pending_mac;
>  	struct net_device *ndev = ndp->ndev.dev;
>  	struct ncsi_rsp_gmcma_pkt *rsp;
> +	unsigned int addr_bytes;
> +	unsigned int payload;
>  	int i;
>  
>  	rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
> +	payload = ncsi_rsp_payload(nr->rsp);
> +	if (payload < sizeof(rsp->address_count) + sizeof(rsp->reserved) +
> +		      sizeof(__be32))

The commit message states it validates the fixed GMCMA fields plus checksum.
Does the NC-SI payload length actually include the 4-byte checksum?

Looking at ncsi_validate_rsp_pkt(), the common.length field does not include
the checksum, but it does include the 2-byte code and 2-byte reason fields.

Is it possible the subtraction of sizeof(__be32) here coincidentally works out
to the same 4 bytes as subtracting the code and reason fields, but conceptually
misrepresents the packet layout?

> +		return -EINVAL;
> +
> +	addr_bytes = payload - sizeof(rsp->address_count) -
> +		     sizeof(rsp->reserved) - sizeof(__be32);
> +	if (rsp->address_count > addr_bytes / ETH_ALEN)
> +		return -EINVAL;
> +
>  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;

This isn't a bug introduced by this patch, but does this read-modify-write
of priv_flags in softirq context race with other modifications?

Since priv_flags is a standard unsigned int, could a concurrent update from
process context holding rtnl_lock (such as modifying IFF_PROMISC) silently
discard this IFF_LIVE_ADDR_CHANGE update or vice versa?

>  
>  	netdev_info(ndev, "NCSI: Received %d provisioned MAC addresses\n",

This isn't a bug introduced by this patch, but are the updates to pending_mac
and gma_flag further down in this function safe from data races?

The caller ncsi_rcv_rsp() explicitly drops ndp->lock before invoking the
response handler:

ncsi_rcv_rsp()
    spin_unlock_irqrestore(&ndp->lock, flags);
    ret = nrh->handler(nr);

Since the NC-SI subsystem allows userspace to inject commands concurrently via
Netlink, could multiple Get MC MAC Address responses process simultaneously on
different CPUs and cause a torn write when copying the 6-byte MAC address into
ndp->pending_mac?

Re: [PATCH net 6/6] net/ncsi: validate GP payload lengths before parsing

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/ncsi: validate GP payload lengths before parsing

This commit bounds MAC and VLAN counts to software and GC-reported limits.
It adds validation to ensure the declared GP payload covers the fixed GP
prefix, consumed entries, and the checksum before parsing filter tables.

> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 94354dca23eac..565d38fd4b92d 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
[ ... ]
> @@ -924,6 +926,14 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
>  	if (rsp->mac_cnt > mac_nbits || rsp->vlan_cnt > ncvf->n_vids)
>  		return -ERANGE;

Does this code read rsp->mac_cnt and rsp->vlan_cnt before validating the
payload length?

If a malformed packet with a payload length of 0 is received,
ncsi_validate_rsp_pkt() guarantees only 20 bytes (the common response
header) are pulled into the skb linear data area.

Since rsp->mac_cnt and rsp->vlan_cnt are at offsets 20 and 24 respectively,
could accessing them prior to the payload length check trigger an
out-of-bounds read?

> +	payload = ncsi_rsp_payload(nr->rsp);
> +	needed = offsetof(struct ncsi_rsp_gp_pkt, mac) - sizeof(rsp->rsp);
> +	needed += mac_cnt * ETH_ALEN;
> +	needed += vlan_cnt * sizeof(__be16);
> +	needed += sizeof(rsp->checksum);
> +	if (payload < needed)
> +		return -EINVAL;

Are there missing timer cancellations during NCSI device unregistration
that could lead to a use-after-free regression?

ncsi_register_dev() initializes timers for 256 NCSI requests. If an NCSI
request is pending when the device is unregistered, and
ncsi_unregister_dev() calls kfree(ndp) without a prior timer_delete_sync(),
could the timer fire later and cause ncsi_request_timeout() to dereference
the freed memory?

Is there a synchronization issue between the workqueue teardown order and
device unregistration?

ncsi_unregister_dev() frees all packages and their channels via
ncsi_remove_package() before calling disable_work_sync(&ndp->work).

If the background task ncsi_dev_work() executes concurrently during this
teardown, could it attempt to suspend or configure the active channel
(e.g., in ncsi_suspend_channel()) and access ndp->active_package and
ndp->active_channel after they have already been freed?

Does the RCU synchronization in the package and channel removal paths properly
prevent use-after-free regressions during receive?

ncsi_remove_channel() and ncsi_remove_package() unlink elements from
RCU-protected lists (list_del_rcu(&nc->node)) and immediately call kfree()
without invoking synchronize_rcu() or using kfree_rcu().

Since the Rx path in ncsi_rcv_rsp() concurrently searches these lists using
list_for_each_entry_rcu(), could an incoming packet cause the Rx handler to
traverse and dereference recently freed package or channel memory?

Does ncsi_rsp_handler_gc() leak the MAC and VLAN filter arrays on repeated Get
Capabilities (GC) responses?

The handler unconditionally allocates memory for nc->mac_filter.addrs and
nc->vlan_filter.vids using kzalloc() and kcalloc(), but does not appear to
check for or free any previously allocated memory.

If a channel goes through a reset cycle via ncsi_reset_dev(), or if a GC
command is explicitly injected via Netlink, could the handler execute again
and overwrite the pointers, leaking the previous allocations?