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

[Simon Horman <horms@kernel.org>]

On Mon, Sep 07, 2026 at 12:37:52AM +0100, Aamir Ahmed wrote:
> The Get Parameters (GP) response handler iterates over MAC address
> and VLAN filter tables using counts (mac_cnt, vlan_cnt) taken directly
> from the response packet.  These counts are not validated against either
> the actual packet length or the filter arrays allocated by the earlier
> Get Capabilities response handler.
> 
> A malformed GP response can cause:
>  - out-of-bounds read past the packet data when iterating pdata
>  - out-of-bounds write to mac_filter.addrs[] when mac_cnt exceeds
>    the allocated filter size (n_uc + n_mc + n_mixed)
>  - out-of-bounds write to vlan_filter.vids[] when vlan_cnt exceeds
>    n_vids
>  - bitmap corruption via set_bit()/clear_bit() beyond u64 width
> 
> Add three validation checks before the filter table loops:
>  1. Packet is large enough for the claimed MAC and VLAN entries
>  2. mac_cnt does not exceed the allocated MAC filter capacity
>  3. vlan_cnt does not exceed the allocated VLAN filter capacity
> 
> Also check for NULL filter arrays in case Get Capabilities was not
> processed first.
> 
> Fixes: 0b49507fc090 ("net/ncsi: Resource management")
> Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
> ---
>  net/ncsi/ncsi-rsp.c | 31 +++++++++++++++++++++++++++++--
>  1 file changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 1401528dbd6c..88f3b8db0289 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -884,10 +884,38 @@ 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);
>  
> +	/* Validate the response carries enough data for the MAC and VLAN
> +	 * tables it claims to contain.  The variable-length area begins
> +	 * at offset 48 (right after the fixed Get Parameters fields).
> +	 */
> +	if (nr->rsp->len < 48 + (unsigned int)rsp->mac_cnt * ETH_ALEN +
> +			    (unsigned int)rsp->vlan_cnt * sizeof(__be16)) {
> +		netdev_warn(ndp->ndev.dev,
> +			    "NCSI: GP response too short for %u MACs + %u VLANs\n",
> +			    rsp->mac_cnt, rsp->vlan_cnt);
> +		return -EINVAL;
> +	}

My concern with this patch is the same as for patch 1/2.

I'm not sure that there is any guarantee that nr->rsp is a linear skb
and if not then checking len is insufficient and I think the check
should be implemented using pskb_may_pull.

Not that any pointers referring to skb data may be invalidated
by that call, so, f.e. rsp should probably be set after the
call to pskb_may_pull().

...


As noted collectively by Greg and Paolo elsewhere:
1. Please include an Assisted-by tag
2. Please keep the number of patches in-flight to 15 or less

-- 
pw-bot: changes-requested