[net] net/ncsi: handle overflow when incrementing mac address

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.

Signed-off-by: Tao Ren <taoren@fb.com>
---
 net/ncsi/ncsi-rsp.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

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");

On 4/22/19 2:54 PM, Jakub Kicinski wrote:
> 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.

Thank you for the quick review Jakub. Sure, I will update the patch description with Fixes tag accordingly.

>> 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).

Make sense. I will split the patch to 2 then: 1) add eth_addr_inc() into linux/etherdevice.h 2) fixes overflow when incrementing mac address by calling eth_addr_inc() function.

> 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");

Thanks for the suggestion. Will add the check.

- Tao