[PATCH net v2 2/2] net: ncsi: validate MAC and VLAN counts in Get Parameters response

[Aamir Ahmed <elb12345@hotmail.co.uk>]

ncsi_rsp_handler_gp() walks the MAC address and VLAN filter tables using
counts taken straight from the response, without checking them against
the data the packet carries or against the filter arrays the earlier Get
Capabilities response sized.

A malformed response can therefore read past the packet, write past
mac_filter.addrs[] when mac_cnt exceeds n_uc + n_mc + n_mixed, write
past vlan_filter.vids[] when vlan_cnt exceeds n_vids, and set bits past
the u64 bitmaps that track the enabled entries - which for both filters
overwrites the array pointer stored just after the bitmap, so the same
loop iteration then writes through it.

The response is not guaranteed to be linear, so make the fixed part
available with pskb_may_pull() before the counts are read, then the
tables they claim, and take the pointer again afterwards because
pskb_may_pull() may have moved the data. Reject the response if either
count exceeds the array it indexes, the bitmap it sets bits in, or if
Get Capabilities has not run and the arrays are absent.

Fixes: 062b3e1b6d4f ("net/ncsi: Refactor MAC, VLAN filters")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
v2:
  - use pskb_may_pull() instead of testing skb->len, in two stages: the
    fixed part before the counts are read, then the tables they claim,
    taking the pointer again afterwards (Simon)
  - also bound both counts by the width of the bitmaps they index; the
    v1 check covered only the arrays, so set_bit()/clear_bit() past the
    u64 could still overwrite the array pointer stored after it
  - correct the Fixes: tag; v1 quoted a hash that does not resolve, and
    the filter arrays came in with the refactor
  - use offsetof() for the start of the tables rather than the literal 48
  - read the counts into locals so the loops do not re-read device data
    across the pull
  - add the Assisted-by: LLM tag (Simon, Greg)
  - name the target tree in the subject
v1: https://lore.kernel.org/netdev/AS8P251MB0001E99A1865DFEC3C3A16E7C8B32@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/

Compile-tested only; I have no NC-SI hardware. The OEM and GMCMA
handlers read past the header at their own device-supplied offsets and
counts and are not covered by either patch; those are separate changes.

 net/ncsi/ncsi-rsp.c | 44 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 5 deletions(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index e4264a028acb..b155b2105b89 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -849,12 +849,20 @@ 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;
+	unsigned char vlan_cnt;
+	unsigned char mac_cnt;
 	unsigned short enable;
 	unsigned char *pdata;
 	unsigned long flags;
 	void *bitmap;
 	int i;
 
+	/* The fixed part of the response has to be present before any of
+	 * its fields, the table counts included, can be read.
+	 */
+	if (!pskb_may_pull(nr->rsp, offsetof(struct ncsi_rsp_gp_pkt, mac)))
+		return -EINVAL;
+
 	/* Find the channel */
 	rsp = (struct ncsi_rsp_gp_pkt *)skb_network_header(nr->rsp);
 	ncsi_find_package_and_channel(ndp, rsp->rsp.common.channel,
@@ -884,13 +892,40 @@ static int ncsi_rsp_handler_gp(struct ncsi_request *nr)
 	nc->modes[NCSI_MODE_AEN].enable = 1;
 	nc->modes[NCSI_MODE_AEN].data[0] = ntohl(rsp->aen_mode);
 
+	/* Make the tables the response claims available, then take the
+	 * pointer again: pskb_may_pull() may have moved the data.
+	 */
+	mac_cnt = rsp->mac_cnt;
+	vlan_cnt = rsp->vlan_cnt;
+	if (!pskb_may_pull(nr->rsp, offsetof(struct ncsi_rsp_gp_pkt, mac) +
+				    mac_cnt * ETH_ALEN +
+				    vlan_cnt * sizeof(__be16)))
+		return -EINVAL;
+
+	rsp = (struct ncsi_rsp_gp_pkt *)skb_network_header(nr->rsp);
+
+	/* The filter tables were sized by the Get Capabilities response,
+	 * whose counts are themselves device supplied, so a larger count
+	 * here would write past them. The counts also index the bitmaps
+	 * that track which entries are enabled, so bound them by those
+	 * as well.
+	 */
+	ncmf = &nc->mac_filter;
+	ncvf = &nc->vlan_filter;
+	if (!ncmf->addrs ||
+	    mac_cnt > ncmf->n_uc + ncmf->n_mc + ncmf->n_mixed ||
+	    mac_cnt > BITS_PER_TYPE(ncmf->bitmap))
+		return -EINVAL;
+	if (!ncvf->vids || vlan_cnt > ncvf->n_vids ||
+	    vlan_cnt > BITS_PER_TYPE(ncvf->bitmap))
+		return -EINVAL;
+
 	/* MAC addresses filter table */
-	pdata = (unsigned char *)rsp + 48;
+	pdata = (unsigned char *)rsp + offsetof(struct ncsi_rsp_gp_pkt, mac);
 	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) {
+	for (i = 0; i < mac_cnt; i++, pdata += 6) {
 		if (!(enable & (0x1 << i)))
 			clear_bit(i, bitmap);
 		else
@@ -902,10 +937,9 @@ 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) {
+	for (i = 0; i < vlan_cnt; i++, pdata += 2) {
 		if (!(enable & (0x1 << i)))
 			clear_bit(i, bitmap);
 		else
-- 
2.55.0