[PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Paolo Pisati]

[partial backport upstream 760db29bdc97b73ff60b091315ad787b1deb5cf5]

Upon invocation, lan78xx_init_mac_address() checks that the mac address present
in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries
to read a new address from an external eeprom or the otp area, and in case both
read fail (or the address read back is invalid), it randomly generates a new
one.

Unfortunately, due to the way the above logic is laid out,
if both read_eeprom() and read_otp() fail, a new mac address is correctly
generated but is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an
incosistent state and with an invalid mac address (e.g. the nic appears to be
completely dead, and doesn't receive any packet, etc):

lan78xx_init_mac_address()
...
if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) {
	if (is_valid_ether_addr(addr) {
		// nop...
	} else {
		random_ether_addr(addr);
	}

	// correctly writes back the new address
	lan78xx_write_reg(RX_ADDRL, addr ...);
	lan78xx_write_reg(RX_ADDRH, addr ...);
} else {
	// XXX if both eeprom and otp read fail, we land here and skip
	// XXX the RX_ADDRL & RX_ADDRH update completely
	random_ether_addr(addr);
}

This bug went unnoticed because lan78xx_read_otp() was buggy itself and would
never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP"
fixed it and as a side effect uncovered this bug.

4.18+ is fine, since the bug was implicitly fixed in 760db29 "lan78xx: Read MAC
address from DT if present" when the address change logic was reorganized, but
it's still present in all stable trees below that: linux-4.4.y, linux-4.9.y,
linux-4.14.y, etc up to linux-4.18.y (not included).

Signed-off-by: Paolo Pisati <p.pisati@gmail.com>
---
 drivers/net/usb/lan78xx.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 50e2e10a..114dc55 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1660,13 +1660,6 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
 				netif_dbg(dev, ifup, dev->net,
 					  "MAC address set to random addr");
 			}
-
-			addr_lo = addr[0] | (addr[1] << 8) |
-				  (addr[2] << 16) | (addr[3] << 24);
-			addr_hi = addr[4] | (addr[5] << 8);
-
-			ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
-			ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
 		} else {
 			/* generate random MAC */
 			random_ether_addr(addr);
@@ -1674,6 +1667,11 @@ static void lan78xx_init_mac_address(struct lan78xx_net *dev)
 				  "MAC address set to random addr");
 		}
 	}
+	addr_lo = addr[0] | (addr[1] << 8) | (addr[2] << 16) | (addr[3] << 24);
+	addr_hi = addr[4] | (addr[5] << 8);
+
+	ret = lan78xx_write_reg(dev, RX_ADDRL, addr_lo);
+	ret = lan78xx_write_reg(dev, RX_ADDRH, addr_hi);
 
 	ret = lan78xx_write_reg(dev, MAF_LO(0), addr_lo);
 	ret = lan78xx_write_reg(dev, MAF_HI(0), addr_hi | MAF_HI_VALID_);
-- 
2.7.4

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Sasha Levin]

On Wed, Nov 07, 2018 at 05:50:57PM +0100, Paolo Pisati wrote:
>[partial backport upstream 760db29bdc97b73ff60b091315ad787b1deb5cf5]
>
>Upon invocation, lan78xx_init_mac_address() checks that the mac address present
>in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries
>to read a new address from an external eeprom or the otp area, and in case both
>read fail (or the address read back is invalid), it randomly generates a new
>one.
>
>Unfortunately, due to the way the above logic is laid out,
>if both read_eeprom() and read_otp() fail, a new mac address is correctly
>generated but is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an
>incosistent state and with an invalid mac address (e.g. the nic appears to be
>completely dead, and doesn't receive any packet, etc):
>
>lan78xx_init_mac_address()
>...
>if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) {
>	if (is_valid_ether_addr(addr) {
>		// nop...
>	} else {
>		random_ether_addr(addr);
>	}
>
>	// correctly writes back the new address
>	lan78xx_write_reg(RX_ADDRL, addr ...);
>	lan78xx_write_reg(RX_ADDRH, addr ...);
>} else {
>	// XXX if both eeprom and otp read fail, we land here and skip
>	// XXX the RX_ADDRL & RX_ADDRH update completely
>	random_ether_addr(addr);
>}
>
>This bug went unnoticed because lan78xx_read_otp() was buggy itself and would
>never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP"
>fixed it and as a side effect uncovered this bug.
>
>4.18+ is fine, since the bug was implicitly fixed in 760db29 "lan78xx: Read MAC
>address from DT if present" when the address change logic was reorganized, but
>it's still present in all stable trees below that: linux-4.4.y, linux-4.9.y,
>linux-4.14.y, etc up to linux-4.18.y (not included).
>
>Signed-off-by: Paolo Pisati <p.pisati@gmail.com>

So why not just take 760db29bdc completely? It looks safer than taking a
partial backport, and will make applying future patches easier.

I tried to do it and it doesn't look like there are any dependencies
that would cause an issue.

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Paolo Pisati]

On Wed, Nov 07, 2018 at 07:17:51PM -0500, Sasha Levin wrote:
> So why not just take 760db29bdc completely? It looks safer than taking a
> partial backport, and will make applying future patches easier.
> 
> I tried to do it and it doesn't look like there are any dependencies
> that would cause an issue.

Somehow i was convinced it didn't build on 4.4.x... can you pick it up?

commit 760db29bdc97b73ff60b091315ad787b1deb5cf5
Author: Phil Elwell <phil@raspberrypi.org>
Date:   Thu Apr 19 17:59:38 2018 +0100

    lan78xx: Read MAC address from DT if present
    
    There is a standard mechanism for locating and using a MAC address from
    the Device Tree. Use this facility in the lan78xx driver to support
    applications without programmed EEPROM or OTP. At the same time,
    regularise the handling of the different address sources.
    
    Signed-off-by: Phil Elwell <phil@raspberrypi.org>
    Signed-off-by: David S. Miller <davem@davemloft.net>
-- 
bye,
p.

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Sasha Levin]

On Thu, Nov 08, 2018 at 12:01:27PM +0100, Paolo Pisati wrote:
>On Wed, Nov 07, 2018 at 07:17:51PM -0500, Sasha Levin wrote:
>> So why not just take 760db29bdc completely? It looks safer than taking a
>> partial backport, and will make applying future patches easier.
>>
>> I tried to do it and it doesn't look like there are any dependencies
>> that would cause an issue.
>
>Somehow i was convinced it didn't build on 4.4.x... can you pick it up?
>
>commit 760db29bdc97b73ff60b091315ad787b1deb5cf5
>Author: Phil Elwell <phil@raspberrypi.org>
>Date:   Thu Apr 19 17:59:38 2018 +0100
>
>    lan78xx: Read MAC address from DT if present
>
>    There is a standard mechanism for locating and using a MAC address from
>    the Device Tree. Use this facility in the lan78xx driver to support
>    applications without programmed EEPROM or OTP. At the same time,
>    regularise the handling of the different address sources.
>
>    Signed-off-by: Phil Elwell <phil@raspberrypi.org>
>    Signed-off-by: David S. Miller <davem@davemloft.net>

Can you confirm it actually works on 4.4?

--
Thanks,
Sasha

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Paolo Pisati]

On Thu, Nov 08, 2018 at 10:49:04AM -0500, Sasha Levin wrote:
> 
> Can you confirm it actually works on 4.4?

Yes, built and tested on 4.4.y:

Tested-by: Paolo Pisati <p.pisati@gmail.com>
-- 
bye,
p.

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Greg KH]

On Fri, Nov 09, 2018 at 12:31:59PM +0100, Paolo Pisati wrote:
> On Thu, Nov 08, 2018 at 10:49:04AM -0500, Sasha Levin wrote:
> > 
> > Can you confirm it actually works on 4.4?
> 
> Yes, built and tested on 4.4.y:
> 
> Tested-by: Paolo Pisati <p.pisati@gmail.com>

Now queued up, thanks.

greg k-h

Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date

[Greg KH]

On Thu, Nov 08, 2018 at 10:49:04AM -0500, Sasha Levin wrote:
> On Thu, Nov 08, 2018 at 12:01:27PM +0100, Paolo Pisati wrote:
> > On Wed, Nov 07, 2018 at 07:17:51PM -0500, Sasha Levin wrote:
> > > So why not just take 760db29bdc completely? It looks safer than taking a
> > > partial backport, and will make applying future patches easier.
> > > 
> > > I tried to do it and it doesn't look like there are any dependencies
> > > that would cause an issue.
> > 
> > Somehow i was convinced it didn't build on 4.4.x... can you pick it up?
> > 
> > commit 760db29bdc97b73ff60b091315ad787b1deb5cf5
> > Author: Phil Elwell <phil@raspberrypi.org>
> > Date:   Thu Apr 19 17:59:38 2018 +0100
> > 
> >    lan78xx: Read MAC address from DT if present
> > 
> >    There is a standard mechanism for locating and using a MAC address from
> >    the Device Tree. Use this facility in the lan78xx driver to support
> >    applications without programmed EEPROM or OTP. At the same time,
> >    regularise the handling of the different address sources.
> > 
> >    Signed-off-by: Phil Elwell <phil@raspberrypi.org>
> >    Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> Can you confirm it actually works on 4.4?

It does not build on 4.4, so I'm dropping it :(

greg k-h