| Message ID | 1541609457-28725-1-git-send-email-p.pisati@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [stable,netdev,4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date | expand |
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. -- Thanks, Sasha
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>
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
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>
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
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
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_);
[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(-)