[PATCH 0/2] net: spider_net: fix possible bitops errors

[PATCH 0/2] net: spider_net: fix possible bitops errors

[Antoine Tenart]

Hi,

Dan reported a possible signedness issue on the pxa168_eth driver. While
having a look at it, I came across a similar problem in the spider_net
driver.

Here is one proposal to fix it. The first patch rework the
spider_net_set_mac() function by removing the spider_net_get_mac_address()
call and using memcpy() to set netdev->dev_addr (which is what's done in
lots of Ethernet drivers) and the second one fix the actual signedness
issue.

If for any reason you really want to keep a call to
spider_net_get_mac_address() because the memcpy() is somehow not good
enough here, we can also come up with a solution involving a temporary
unsigned char variable.

I couldn't test these changes, so please do.

Thanks,

Antoine

Antoine Tenart (2):
  net: spider_net: do not read mac address again after setting it
  net: spider_net: avoid using signed char for bitops

 drivers/net/ethernet/toshiba/spider_net.c | 42 ++++---------------------------
 1 file changed, 5 insertions(+), 37 deletions(-)

-- 
1.9.1

[PATCH] net: pxa168_eth: avoid using signed char for bitops

[Antoine Tenart]

Signedness bugs may occur when using signed char for bitops,
depending on if the highest bit is ever used.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
---

Dan reported a static checker warning:
http://marc.info/?l=kernel-janitors&m=141218246222535&w=2

This patch fixes it. Instead of using sa->sa_data we now use
dev->dev_addr (of type unsigned char *) to avoid possible
signedness issues.

 drivers/net/ethernet/marvell/pxa168_eth.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
index 24de41231593..c3b209cd0660 100644
--- a/drivers/net/ethernet/marvell/pxa168_eth.c
+++ b/drivers/net/ethernet/marvell/pxa168_eth.c
@@ -634,12 +634,12 @@ static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
 	memcpy(oldMac, dev->dev_addr, ETH_ALEN);
 	memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
 
-	mac_h = sa->sa_data[0] << 24;
-	mac_h |= sa->sa_data[1] << 16;
-	mac_h |= sa->sa_data[2] << 8;
-	mac_h |= sa->sa_data[3];
-	mac_l = sa->sa_data[4] << 8;
-	mac_l |= sa->sa_data[5];
+	mac_h = dev->dev_addr[0] << 24;
+	mac_h |= dev->dev_addr[1] << 16;
+	mac_h |= dev->dev_addr[2] << 8;
+	mac_h |= dev->dev_addr[3];
+	mac_l = dev->dev_addr[4] << 8;
+	mac_l |= dev->dev_addr[5];
 	wrl(pep, MAC_ADDR_HIGH, mac_h);
 	wrl(pep, MAC_ADDR_LOW, mac_l);
 
-- 
1.9.1

[PATCH 1/2] net: spider_net: do not read mac address again after setting it

[Antoine Tenart]

This patch removes the spider_net_get_mac_address() call at the end of
the spider_net_set_mac() function. The dev->dev_addr is instead updated
with a memcpy() from sa->sa_data.

Since spider_net_get_mac_address() is not used anywhere else, this patch
also removes the function.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/toshiba/spider_net.c | 36 ++-----------------------------
 1 file changed, 2 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/spider_net.c b/drivers/net/ethernet/toshiba/spider_net.c
index 3e38f67c6011..713313e15c68 100644
--- a/drivers/net/ethernet/toshiba/spider_net.c
+++ b/drivers/net/ethernet/toshiba/spider_net.c
@@ -267,34 +267,6 @@ spider_net_set_promisc(struct spider_net_card *card)
 }
 
 /**
- * spider_net_get_mac_address - read mac address from spider card
- * @card: device structure
- *
- * reads MAC address from GMACUNIMACU and GMACUNIMACL registers
- */
-static int
-spider_net_get_mac_address(struct net_device *netdev)
-{
-	struct spider_net_card *card = netdev_priv(netdev);
-	u32 macl, macu;
-
-	macl = spider_net_read_reg(card, SPIDER_NET_GMACUNIMACL);
-	macu = spider_net_read_reg(card, SPIDER_NET_GMACUNIMACU);
-
-	netdev->dev_addr[0] = (macu >> 24) & 0xff;
-	netdev->dev_addr[1] = (macu >> 16) & 0xff;
-	netdev->dev_addr[2] = (macu >> 8) & 0xff;
-	netdev->dev_addr[3] = macu & 0xff;
-	netdev->dev_addr[4] = (macl >> 8) & 0xff;
-	netdev->dev_addr[5] = macl & 0xff;
-
-	if (!is_valid_ether_addr(&netdev->dev_addr[0]))
-		return -EINVAL;
-
-	return 0;
-}
-
-/**
  * spider_net_get_descr_status -- returns the status of a descriptor
  * @descr: descriptor to look at
  *
@@ -1345,6 +1317,8 @@ spider_net_set_mac(struct net_device *netdev, void *p)
 	if (!is_valid_ether_addr(addr->sa_data))
 		return -EADDRNOTAVAIL;
 
+	memcpy(netdev->dev_addr, addr->sa_data, ETH_ALEN);
+
 	/* switch off GMACTPE and GMACRPE */
 	regvalue = spider_net_read_reg(card, SPIDER_NET_GMACOPEMD);
 	regvalue &= ~((1 << 5) | (1 << 6));
@@ -1364,12 +1338,6 @@ spider_net_set_mac(struct net_device *netdev, void *p)
 
 	spider_net_set_promisc(card);
 
-	/* look up, whether we have been successful */
-	if (spider_net_get_mac_address(netdev))
-		return -EADDRNOTAVAIL;
-	if (memcmp(netdev->dev_addr,addr->sa_data,netdev->addr_len))
-		return -EADDRNOTAVAIL;
-
 	return 0;
 }
 
-- 
1.9.1

[PATCH 2/2] net: spider_net: avoid using signed char for bitops

[Antoine Tenart]

Signedness bugs may occur when using signed char for bitops,
depending on if the highest bit is ever used.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/net/ethernet/toshiba/spider_net.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/toshiba/spider_net.c b/drivers/net/ethernet/toshiba/spider_net.c
index 713313e15c68..8e9371a3388a 100644
--- a/drivers/net/ethernet/toshiba/spider_net.c
+++ b/drivers/net/ethernet/toshiba/spider_net.c
@@ -1325,9 +1325,9 @@ spider_net_set_mac(struct net_device *netdev, void *p)
 	spider_net_write_reg(card, SPIDER_NET_GMACOPEMD, regvalue);
 
 	/* write mac */
-	macu = (addr->sa_data[0]<<24) + (addr->sa_data[1]<<16) +
-		(addr->sa_data[2]<<8) + (addr->sa_data[3]);
-	macl = (addr->sa_data[4]<<8) + (addr->sa_data[5]);
+	macu = (netdev->dev_addr[0]<<24) + (netdev->dev_addr[1]<<16) +
+		(netdev->dev_addr[2]<<8) + (netdev->dev_addr[3]);
+	macl = (netdev->dev_addr[4]<<8) + (netdev->dev_addr[5]);
 	spider_net_write_reg(card, SPIDER_NET_GMACUNIMACU, macu);
 	spider_net_write_reg(card, SPIDER_NET_GMACUNIMACL, macl);
 
-- 
1.9.1

Re: [PATCH] net: pxa168_eth: avoid using signed char for bitops

[Antoine Tenart]

On Fri, Oct 03, 2014 at 05:01:54PM +0200, Antoine Tenart wrote:
> Signedness bugs may occur when using signed char for bitops,
> depending on if the highest bit is ever used.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> 
> Dan reported a static checker warning:
> http://marc.info/?l=kernel-janitors&m=141218246222535&w=2
> 
> This patch fixes it. Instead of using sa->sa_data we now use
> dev->dev_addr (of type unsigned char *) to avoid possible
> signedness issues.

Well, this patch shouldn't be in this thread. I'm resending it.
Sorry for the noise.

Antoine

> 
>  drivers/net/ethernet/marvell/pxa168_eth.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/pxa168_eth.c b/drivers/net/ethernet/marvell/pxa168_eth.c
> index 24de41231593..c3b209cd0660 100644
> --- a/drivers/net/ethernet/marvell/pxa168_eth.c
> +++ b/drivers/net/ethernet/marvell/pxa168_eth.c
> @@ -634,12 +634,12 @@ static int pxa168_eth_set_mac_address(struct net_device *dev, void *addr)
>  	memcpy(oldMac, dev->dev_addr, ETH_ALEN);
>  	memcpy(dev->dev_addr, sa->sa_data, ETH_ALEN);
>  
> -	mac_h = sa->sa_data[0] << 24;
> -	mac_h |= sa->sa_data[1] << 16;
> -	mac_h |= sa->sa_data[2] << 8;
> -	mac_h |= sa->sa_data[3];
> -	mac_l = sa->sa_data[4] << 8;
> -	mac_l |= sa->sa_data[5];
> +	mac_h = dev->dev_addr[0] << 24;
> +	mac_h |= dev->dev_addr[1] << 16;
> +	mac_h |= dev->dev_addr[2] << 8;
> +	mac_h |= dev->dev_addr[3];
> +	mac_l = dev->dev_addr[4] << 8;
> +	mac_l |= dev->dev_addr[5];
>  	wrl(pep, MAC_ADDR_HIGH, mac_h);
>  	wrl(pep, MAC_ADDR_LOW, mac_l);
>  
> -- 
> 1.9.1
> 

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 2/2] net: spider_net: avoid using signed char for bitops

[Arnd Bergmann]

On Friday 03 October 2014 17:01:56 Antoine Tenart wrote:
> Signedness bugs may occur when using signed char for bitops,
> depending on if the highest bit is ever used.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> 

It's probably worth mentioning here that this cannot happen in this
driver because it's only used in one powerpc-specific chip, and 'char'
is always 'unsigned' on powerpc.

	Arnd

Re: [PATCH 2/2] net: spider_net: avoid using signed char for bitops

[Antoine Tenart]

Hi Arnd,

On Fri, Oct 03, 2014 at 05:15:05PM +0200, Arnd Bergmann wrote:
> On Friday 03 October 2014 17:01:56 Antoine Tenart wrote:
> > Signedness bugs may occur when using signed char for bitops,
> > depending on if the highest bit is ever used.
> > 
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > 
> 
> It's probably worth mentioning here that this cannot happen in this
> driver because it's only used in one powerpc-specific chip, and 'char'
> is always 'unsigned' on powerpc.

Oh, that's good to know :)


Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/2] net: spider_net: fix possible bitops errors

[David Miller]

From: Antoine Tenart <antoine.tenart@free-electrons.com>
Date: Fri,  3 Oct 2014 17:01:53 +0200

> Dan reported a possible signedness issue on the pxa168_eth driver. While
> having a look at it, I came across a similar problem in the spider_net
> driver.
> 
> Here is one proposal to fix it. The first patch rework the
> spider_net_set_mac() function by removing the spider_net_get_mac_address()
> call and using memcpy() to set netdev->dev_addr (which is what's done in
> lots of Ethernet drivers) and the second one fix the actual signedness
> issue.
> 
> If for any reason you really want to keep a call to
> spider_net_get_mac_address() because the memcpy() is somehow not good
> enough here, we can also come up with a solution involving a temporary
> unsigned char variable.

Series applied, thanks.