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

[Jakub Kicinski <kuba@kernel.org>]

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?