re: net: Micrel KSZ8841/2 PCI Ethernet driver

re: net: Micrel KSZ8841/2 PCI Ethernet driver

[Dan Carpenter]

Hello Tristram,

I was going through some old stuff and I had a question about ksz884x.c.

The patch 8ca86fd83eae: "net: Micrel KSZ8841/2 PCI Ethernet driver" 
from Feb 8, 2010, leads to the following warning:
drivers/net/ethernet/micrel/ksz884x.c:5678 netdev_set_mac_address()
	 error: memcpy() 'mac->sa_data' too small (14 vs 32)

  5663  static int netdev_set_mac_address(struct net_device *dev, void *addr)
  5664  {
  5665          struct dev_priv *priv = netdev_priv(dev);
  5666          struct dev_info *hw_priv = priv->adapter;
  5667          struct ksz_hw *hw = &hw_priv->hw;
  5668          struct sockaddr *mac = addr;
  5669          uint interrupt;
  5670  
  5671          if (priv->port.first_port > 0)
  5672                  hw_del_addr(hw, dev->dev_addr);
  5673          else {
  5674                  hw->mac_override = 1;
  5675                  memcpy(hw->override_addr, mac->sa_data, ETH_ALEN);
  5676          }
  5677  
  5678          memcpy(dev->dev_addr, mac->sa_data, MAX_ADDR_LEN);
                                      ^^^^^^^^^^^^
This has only 14 bytes so we're copying bogus data from beyond the end
of the struct into dev->dev_addr.

  5679  

regards,
dan carpenter

re: net: Micrel KSZ8841/2 PCI Ethernet driver

[Ben Hutchings]

On Wed, 2012-04-18 at 09:47 +0300, Dan Carpenter wrote:
> Hello Tristram,
> 
> I was going through some old stuff and I had a question about ksz884x.c.
> 
> The patch 8ca86fd83eae: "net: Micrel KSZ8841/2 PCI Ethernet driver" 
> from Feb 8, 2010, leads to the following warning:
> drivers/net/ethernet/micrel/ksz884x.c:5678 netdev_set_mac_address()
> 	 error: memcpy() 'mac->sa_data' too small (14 vs 32)
> 
>   5663  static int netdev_set_mac_address(struct net_device *dev, void *addr)
>   5664  {
>   5665          struct dev_priv *priv = netdev_priv(dev);
>   5666          struct dev_info *hw_priv = priv->adapter;
>   5667          struct ksz_hw *hw = &hw_priv->hw;
>   5668          struct sockaddr *mac = addr;
>   5669          uint interrupt;
>   5670  
>   5671          if (priv->port.first_port > 0)
>   5672                  hw_del_addr(hw, dev->dev_addr);
>   5673          else {
>   5674                  hw->mac_override = 1;
>   5675                  memcpy(hw->override_addr, mac->sa_data, ETH_ALEN);
>   5676          }
>   5677  
>   5678          memcpy(dev->dev_addr, mac->sa_data, MAX_ADDR_LEN);
>                                       ^^^^^^^^^^^^
> This has only 14 bytes so we're copying bogus data from beyond the end
> of the struct into dev->dev_addr.

The correct length is ETH_ALEN, as for the previous memcpy().

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: net: Micrel KSZ8841/2 PCI Ethernet driver

[Dan Carpenter]

On Wed, Apr 18, 2012 at 03:13:14PM +0100, Ben Hutchings wrote:
> 
> The correct length is ETH_ALEN, as for the previous memcpy().

Thanks.  I'll send a patch for that.

regards,
dan carpenter

[patch] ksz884x: don't copy too much in netdev_set_mac_address()

[Dan Carpenter]

MAX_ADDR_LEN is 32.  ETH_ALEN is 6.  mac->sa_data is a 14 byte array, so
the memcpy() is doing a read past the end of the array.  I asked about
this on netdev and Ben Hutchings told me it's supposed to be copying
ETH_ALEN bytes (thanks Ben).

Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

diff --git a/drivers/net/ethernet/micrel/ksz884x.c b/drivers/net/ethernet/micrel/ksz884x.c
index ef723b1..eaf9ff0 100644
--- a/drivers/net/ethernet/micrel/ksz884x.c
+++ b/drivers/net/ethernet/micrel/ksz884x.c
@@ -5675,7 +5675,7 @@ static int netdev_set_mac_address(struct net_device *dev, void *addr)
 		memcpy(hw->override_addr, mac->sa_data, ETH_ALEN);
 	}
 
-	memcpy(dev->dev_addr, mac->sa_data, MAX_ADDR_LEN);
+	memcpy(dev->dev_addr, mac->sa_data, ETH_ALEN);
 
 	interrupt = hw_block_intr(hw);
 

Re: [patch] ksz884x: don't copy too much in netdev_set_mac_address()

[David Miller]

From: Dan Carpenter <dan.carpenter@oracle.com>
Date: Thu, 19 Apr 2012 10:00:19 +0300

> MAX_ADDR_LEN is 32.  ETH_ALEN is 6.  mac->sa_data is a 14 byte array, so
> the memcpy() is doing a read past the end of the array.  I asked about
> this on netdev and Ben Hutchings told me it's supposed to be copying
> ETH_ALEN bytes (thanks Ben).
> 
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>

Applied, thanks Dan.