* [PATCH net] net/ncsi: handle overflow when incrementing mac address
@ 2019-04-22 17:27 Tao Ren
2019-04-22 21:54 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Tao Ren @ 2019-04-22 17:27 UTC (permalink / raw)
To: Samuel Mendoza-Jonas, David S . Miller, netdev, linux-kernel,
Joel Stanley, Andrew Jeffery, openbmc
Cc: Tao Ren
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(-)
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);
ret = ops->ndo_set_mac_address(ndev, &saddr);
if (ret < 0)
netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH net] net/ncsi: handle overflow when incrementing mac address
2019-04-22 17:27 [PATCH net] net/ncsi: handle overflow when incrementing mac address Tao Ren
@ 2019-04-22 21:54 ` Jakub Kicinski
2019-04-22 22:38 ` Tao Ren
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2019-04-22 21:54 UTC (permalink / raw)
To: Tao Ren
Cc: Samuel Mendoza-Jonas, David S . Miller, netdev, linux-kernel,
Joel Stanley, Andrew Jeffery, openbmc
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");
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH net] net/ncsi: handle overflow when incrementing mac address
2019-04-22 21:54 ` Jakub Kicinski
@ 2019-04-22 22:38 ` Tao Ren
0 siblings, 0 replies; 3+ messages in thread
From: Tao Ren @ 2019-04-22 22:38 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Samuel Mendoza-Jonas, David S . Miller, netdev, linux-kernel,
Joel Stanley, Andrew Jeffery, openbmc
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-04-22 22:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-22 17:27 [PATCH net] net/ncsi: handle overflow when incrementing mac address Tao Ren
2019-04-22 21:54 ` Jakub Kicinski
2019-04-22 22:38 ` Tao Ren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).