lkml.org 
[lkml]   [2019]   [Apr]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH net] net/ncsi: handle overflow when incrementing mac address
Date
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
\
 
 \ /
  Last update: 2019-04-23 00:39    [W:0.049 / U:3.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site