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?