All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: Paolo Pisati <p.pisati@gmail.com>
Cc: Woojung Huh <woojung.huh@microchip.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	netdev@vger.kernel.org, stable@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [stable,netdev,4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
Date: Wed, 7 Nov 2018 19:17:51 -0500	[thread overview]
Message-ID: <20181108001751.GA8097@sasha-vm> (raw)

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

WARNING: multiple messages have this Message-ID (diff)
From: Sasha Levin <sashal@kernel.org>
To: Paolo Pisati <p.pisati@gmail.com>
Cc: Woojung Huh <woojung.huh@microchip.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	netdev@vger.kernel.org, stable@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
Date: Wed, 7 Nov 2018 19:17:51 -0500	[thread overview]
Message-ID: <20181108001751.GA8097@sasha-vm> (raw)
In-Reply-To: <1541609457-28725-1-git-send-email-p.pisati@gmail.com>

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

WARNING: multiple messages have this Message-ID (diff)
From: Sasha Levin <sashal@kernel.org>
To: Paolo Pisati <p.pisati@gmail.com>
Cc: Woojung Huh <woojung.huh@microchip.com>,
	Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>,
	netdev@vger.kernel.org, stable@vger.kernel.org,
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
Date: Wed, 7 Nov 2018 19:17:51 -0500	[thread overview]
Message-ID: <20181108001751.GA8097@sasha-vm> (raw)
In-Reply-To: <1541609457-28725-1-git-send-email-p.pisati@gmail.com>

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.

             reply	other threads:[~2018-11-08  0:17 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-11-08  0:17 Sasha Levin [this message]
2018-11-08  0:17 ` [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date Sasha Levin
2018-11-08  0:17 ` Sasha Levin
  -- strict thread matches above, loose matches on Subject: below --
2018-11-29 13:30 [stable,netdev,4.4+] " Greg Kroah-Hartman
2018-11-29 13:30 ` [PATCH] [stable, netdev 4.4+] " Greg KH
2018-11-29 12:31 [stable,netdev,4.4+] " Greg KH
2018-11-29 12:31 ` [PATCH] [stable, netdev 4.4+] " Greg KH
2018-11-09 11:31 [stable,netdev,4.4+] " Paolo Pisati
2018-11-09 11:31 ` [PATCH] [stable, netdev 4.4+] " Paolo Pisati
2018-11-08 15:49 [stable,netdev,4.4+] " Sasha Levin
2018-11-08 15:49 ` [PATCH] [stable, netdev 4.4+] " Sasha Levin
2018-11-08 11:01 [stable,netdev,4.4+] " Paolo Pisati
2018-11-08 11:01 ` [PATCH] [stable, netdev 4.4+] " Paolo Pisati
2018-11-07 16:50 [stable,netdev,4.4+] " Paolo Pisati
2018-11-07 16:50 ` [PATCH] [stable, netdev 4.4+] " Paolo Pisati

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181108001751.GA8097@sasha-vm \
    --to=sashal@kernel.org \
    --cc=UNGLinuxDriver@microchip.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=p.pisati@gmail.com \
    --cc=stable@vger.kernel.org \
    --cc=woojung.huh@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.