Messages in this thread | | | Date | Mon, 22 Apr 2019 14:54:43 -0700 | From | Jakub Kicinski <> | Subject | Re: [PATCH net] net/ncsi: handle overflow when incrementing mac address |
| |
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");
| |