Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 22 Apr 2019 14:54:43 -0700
From:   Jakub Kicinski <jakub.kicinski@netronome.com>
To:     Tao Ren <taoren@fb.com>
Cc:     Samuel Mendoza-Jonas <sam@mendozajonas.com>,
        "David S . Miller" <davem@davemloft.net>, <netdev@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, Joel Stanley <joel@jms.id.au>,
        Andrew Jeffery <andrew@aj.id.au>, <openbmc@lists.ozlabs.org>
Subject: Re: [PATCH net] net/ncsi: handle overflow when incrementing mac
 address
Message-ID: <20190422145443.6391eaf8@cakuba.netronome.com>
In-Reply-To: <20190422172754.1011894-1-taoren@fb.com>
References: <20190422172754.1011894-1-taoren@fb.com>
Organization: Netronome Systems, Ltd.
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 22 Apr 2019 10:27:54 -0700, Tao Ren wrote:
> Previously BMC's MAC address is calculated by simply adding 1 to the
> last byte of network controller's MAC address, and it produces incorrect
> result when network controller's MAC address ends with 0xFF.
> The problem is fixed by detecting integer overflow when incrementing MAC
> address and adding the carry bit (if any) to the next/left bytes of the
> MAC address.
> 

It'd be good to have a Fixes tag, if it's worth going to the net tree.

> Signed-off-by: Tao Ren <taoren@fb.com>
> ---
>  net/ncsi/ncsi-rsp.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index dc07fcc7938e..eb42bbdb7501 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -658,7 +658,8 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>  	const struct net_device_ops *ops = ndev->netdev_ops;
>  	struct ncsi_rsp_oem_pkt *rsp;
>  	struct sockaddr saddr;
> -	int ret = 0;
> +	int ret, offset;
> +	u16 carry = 1;
>  
>  	/* Get the response header */
>  	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> @@ -667,7 +668,12 @@ static int ncsi_rsp_handler_oem_bcm_gma(struct ncsi_request *nr)
>  	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>  	memcpy(saddr.sa_data, &rsp->data[BCM_MAC_ADDR_OFFSET], ETH_ALEN);
>  	/* Increase mac address by 1 for BMC's address */
> -	saddr.sa_data[ETH_ALEN - 1]++;
> +	offset = ETH_ALEN - 1;
> +	do {
> +		carry += (u8)saddr.sa_data[offset];
> +		saddr.sa_data[offset] = (char)carry;
> +		carry = carry >> 8;
> +	} while (carry != 0 && --offset >= 0);

We have eth_addr_dec(), perhaps it'd be good to add an eth_addr_inc()
equivalent?  (I'm not sure if it'd have to be in net-next, it's a tiny
function, and OK for net for my taste, but I had been wrong before).

If I'm allowed to be paranoid I'd also advise checking the resulting
MAC is a valid ethernet unicast addr.

>  	ret = ops->ndo_set_mac_address(ndev, &saddr);
>  	if (ret < 0)
>  		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");