[PATCH net 0/4] tg3: Bug fixes

[PATCH net 0/4] tg3: Bug fixes

[Pavan Chebbi]

This patchset has 4 fixes.  The first patch restores probe on 57766 when
the 31-bit coherent DMA mask cannot be set.  The second patch makes NCSI
PLL lock on the TLP clock to fix WOL at 1G speed on 5720.  The third
patch fixes ethtool -i display on 5719/5720 when NCSI component is
missing.  The fourth one fixes a possible heap overflow in the NVRAM
write path.

Pavan Chebbi (3):
  tg3: Don't fail probe if 31-bit coherent DMA mask is unsupported
  tg3: Select PLL clock when preparing for shutdown
  tg3: Skip querying for management FW when no NCSI present

Shravya KN (1):
  tg3: Fix heap overflow in unbuffered NVRAM write path

 drivers/net/ethernet/broadcom/tg3.c | 48 ++++++++++++++++++++++++-----
 drivers/net/ethernet/broadcom/tg3.h |  6 +++-
 2 files changed, 45 insertions(+), 9 deletions(-)

-- 
2.52.0

[PATCH net 1/4] tg3: Don't fail probe if 31-bit coherent DMA mask is unsupported

[Pavan Chebbi]

Commit 614f4d166eee ("tg3: Set coherent DMA mask bits to 31 for
BCM57766 chipsets") lowered the coherent DMA mask to 31 bits as a
workaround for a spurious address overflow error in the hardware.
The result of dma_set_coherent_mask() is fatal to probe, so on
platforms that cannot satisfy such a mask the device fails to probe:

  tg3 0000:01:00.0: Unable to obtain 64 bit DMA for consistent allocations
  tg3 0000:01:00.0: probe with driver tg3 failed with error -5

This was reported on a Mac mini (Macmini6,2, BCM57766) running a
kernel built without CONFIG_ZONE_DMA and with IOMMU passthrough as
the default. Without CONFIG_ZONE_DMA, dma_direct_supported() only
accepts a mask narrower than 32 bits if it covers the highest
physical memory address, so with direct mapping a 31-bit coherent
mask is rejected whenever memory extends above 2GB.

Treat the 31-bit mask as best effort.  If it is refused, warn and fall
back to the mask used before 614f4d166eee, so that the device still
probes.  Platforms that accept the mask are unaffected.  Where it is
refused, 614f4d166eee never protected the device, it only stopped it
from probing, so this restores the behaviour of kernels before that
commit.  Warn when a status block lands at an address matching the
pattern described in 614f4d166eee, so that a resulting TX timeout can
be identified.

Fixes: 614f4d166eee ("tg3: Set coherent DMA mask bits to 31 for BCM57766 chipsets")
Reported-by: Rui Salvaterra <rsalvaterra@gmail.com>
Link: https://lore.kernel.org/netdev/CALjTZvYKHWrD5m+RXimjxODvpFPw7Cq_EOEuzRi1PZT9_JxF+g@mail.gmail.com/
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 95827df4cf6a..2cf9d2ea069b 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -8867,6 +8867,13 @@ static int tg3_alloc_consistent(struct tg3 *tp)
 		if (!tnapi->hw_status)
 			goto err_out;
 
+		if (tg3_asic_rev(tp) == ASIC_REV_57766 &&
+		    (lower_32_bits(tnapi->status_mapping) & 0xffff0000) ==
+		    0xffff0000)
+			dev_warn(&tp->pdev->dev,
+				 "Status block at DMA address %pad may cause TX timeouts on this device\n",
+				 &tnapi->status_mapping);
+
 		sblk = tnapi->hw_status;
 
 		if (tg3_flag(tp, ENABLE_RSS)) {
@@ -17756,6 +17763,7 @@ static int tg3_init_one(struct pci_dev *pdev,
 	u64 dma_mask, persist_dma_mask;
 	netdev_features_t features = 0;
 	u8 addr[ETH_ALEN] __aligned(2);
+	u64 test_dma_mask = 0;
 
 	err = pci_enable_device(pdev);
 	if (err) {
@@ -17895,15 +17903,22 @@ static int tg3_init_one(struct pci_dev *pdev,
 		persist_dma_mask = dma_mask = DMA_BIT_MASK(64);
 
 	if (tg3_asic_rev(tp) == ASIC_REV_57766)
-		persist_dma_mask = DMA_BIT_MASK(31);
+		test_dma_mask = DMA_BIT_MASK(31);
 
 	/* Configure DMA attributes. */
 	if (dma_mask > DMA_BIT_MASK(32)) {
 		err = dma_set_mask(&pdev->dev, dma_mask);
 		if (!err) {
 			features |= NETIF_F_HIGHDMA;
-			err = dma_set_coherent_mask(&pdev->dev,
-						    persist_dma_mask);
+			if (!test_dma_mask ||
+			    dma_set_coherent_mask(&pdev->dev,
+						  test_dma_mask)) {
+				if (test_dma_mask)
+					dev_warn(&pdev->dev,
+						 "Unable to set 31 bit coherent DMA mask, workaround not applied\n");
+				err = dma_set_coherent_mask(&pdev->dev,
+							    persist_dma_mask);
+			}
 			if (err < 0) {
 				dev_err(&pdev->dev, "Unable to obtain 64 bit "
 					"DMA for consistent allocations\n");
-- 
2.52.0

[PATCH net 2/4] tg3: Select PLL clock when preparing for shutdown

[Pavan Chebbi]

When the 5720 was linked up at 1G, it may fail to wake up on a
single Magic packet.  This is a hardware issue, which can be avoided
by locking the PLL clock source before shutdown.

When preparing for power down with Wake-on-LAN enabled, if the link was
up at 1G, set CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL in TG3_CPMU_PLL_CTRL_STAT,
so that the NCSI PLL locks on the TLP clock.  The setting is cleared on
the next chip reset.

This was reported by a user, who confirmed that this change fixes it.

Fixes: ba1f3c76d760 ("tg3: Enable 5720 support")
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Kalesh AP <kalesh-anakkur.purayil@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 14 ++++++++++++++
 drivers/net/ethernet/broadcom/tg3.h |  6 +++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 2cf9d2ea069b..e9b48e58fdd4 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -4127,6 +4127,14 @@ static void tg3_power_down_prepare(struct tg3 *tp)
 	if (device_should_wake) {
 		u32 mac_mode;
 
+		if (tg3_asic_rev(tp) == ASIC_REV_5720 &&
+		    tp->link_config.active_speed == SPEED_1000) {
+			u32 val = tr32(TG3_CPMU_PLL_CTRL_STAT);
+
+			tw32(TG3_CPMU_PLL_CTRL_STAT,
+			     val | CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL);
+		}
+
 		if (!(tp->phy_flags & TG3_PHYFLG_PHY_SERDES)) {
 			if (do_low_power &&
 			    !(tp->phy_flags & TG3_PHYFLG_IS_FET)) {
@@ -9394,6 +9402,12 @@ static int tg3_chip_reset(struct tg3 *tp)
 
 	tg3_restore_clk(tp);
 
+	if (tg3_asic_rev(tp) == ASIC_REV_5720) {
+		val = tr32(TG3_CPMU_PLL_CTRL_STAT);
+		tw32(TG3_CPMU_PLL_CTRL_STAT,
+		     val & ~CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL);
+	}
+
 	/* Increase the core clock speed to fix tx timeout issue for 5762
 	 * with 100Mbps link speed.
 	 */
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index a9e7f88fa26d..631bacc515ab 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -1168,7 +1168,11 @@
 #define  CPMU_CLCK_STAT_MAC_CLCK_62_5	 0x00000000
 #define  CPMU_CLCK_STAT_MAC_CLCK_12_5	 0x00110000
 #define  CPMU_CLCK_STAT_MAC_CLCK_6_25	 0x00130000
-/* 0x3634 --> 0x365c unused */
+/* 0x3634 unused */
+
+#define TG3_CPMU_PLL_CTRL_STAT		0x00003638
+#define  CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL	0x04000000
+/* 0x363c --> 0x365c unused */
 
 #define TG3_CPMU_MUTEX_REQ		0x0000365c
 #define  CPMU_MUTEX_REQ_DRIVER		 0x00001000
-- 
2.52.0

Re: [PATCH net 2/4] tg3: Select PLL clock when preparing for shutdown

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In tg3_power_down_prepare()
  (drivers/net/ethernet/broadcom/tg3.c, around lines 4130-4136), the new…
- [Medium] The patch does an unlocked read-modify-write of
  TG3_CPMU_PLL_CTRL_STAT (0x3638) that sets…

--

tg3: Select PLL clock when preparing for shutdown

The patch works around a 5720 hardware issue where the chip may fail to
wake on a single magic packet after being linked at 1G. When WoL is
enabled and active_speed is SPEED_1000, tg3_power_down_prepare() sets
CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL in the new TG3_CPMU_PLL_CTRL_STAT register.
tg3_chip_reset() clears the bit on every 5720 reset.

> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 2cf9d2ea069b5..e9b48e58fdd45 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -4127,6 +4127,14 @@ static void tg3_power_down_prepare(struct tg3 *tp)
>  	if (device_should_wake) {
>  		u32 mac_mode;
>  
> +		if (tg3_asic_rev(tp) == ASIC_REV_5720 &&
> +		    tp->link_config.active_speed == SPEED_1000) {

[Severity: Medium]
Does active_speed here describe the link the chip will be using while it
waits for the magic packet?

The 5720 does not use phylib. Earlier in tg3_power_down_prepare() the
driver sets TG3_PHYFLG_IS_LOW_POWER and calls tg3_setup_phy(tp, false).
In tg3_setup_copper_phy(), active_speed is only written when the link is
currently up:

    tp->link_config.active_speed = current_speed;

That write is inside the BMSR_LSTATUS branch. At relink,
tg3_phy_copper_begin() then writes a new WoL advertisement and restarts
autoneg, unless KEEP_LINK_ON_PWRDN is set:

    if ((tp->phy_flags & TG3_PHYFLG_IS_LOW_POWER) &&
        !(tp->phy_flags & TG3_PHYFLG_KEEP_LINK_ON_PWRDN)) {
        adv = ADVERTISED_10baseT_Half |
              ADVERTISED_10baseT_Full;
        ...
        if (tp->phy_flags & TG3_PHYFLG_1G_ON_VAUX_OK) {
            ...
            adv |= ADVERTISED_1000baseT_Full;
        }

Nothing updates active_speed after that, so this check reads the speed
from before the WoL renegotiation, or a stale value. A few cases look
possible:

  - 1G_ON_VAUX_OK is set, and the link was at 100M before shutdown (for
    example, limited with ethtool) or was down. The PHY renegotiates to
    1G on Vaux, but the PLL bit is never set.

  - KEEP_LINK_ON_PWRDN is set, the cable was unplugged at shutdown, and a
    1G link comes up later during WoL. active_speed is stale, so the bit
    is not set.

  - Neither flag is set, so the WoL link drops to 10/100. The bit is set
    anyway because active_speed is still 1000 from earlier.

The existing active_speed check further down this block, for GMII vs
MII, is guarded by KEEP_LINK_ON_PWRDN, where the link is kept as is. This
new check has no such guard and does not look at tp->link_up.

The commit message says "When the 5720 was linked up at 1G". Does that
mean the link before shutdown or the link during WoL? If it means the WoL
link, could the first two cases still hit the reported wake failure?

> +			u32 val = tr32(TG3_CPMU_PLL_CTRL_STAT);
> +
> +			tw32(TG3_CPMU_PLL_CTRL_STAT,
> +			     val | CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL);
> +		}
> +

[ ... ]

> @@ -9394,6 +9402,12 @@ static int tg3_chip_reset(struct tg3 *tp)
>  
>  	tg3_restore_clk(tp);
>  
> +	if (tg3_asic_rev(tp) == ASIC_REV_5720) {
> +		val = tr32(TG3_CPMU_PLL_CTRL_STAT);
> +		tw32(TG3_CPMU_PLL_CTRL_STAT,
> +		     val & ~CPMU_PLL_TLP_CLCK_SRC_NCSI_PLL);
> +	}

[Severity: Medium]
Is TG3_CPMU_PLL_CTRL_STAT per-function, or is it shared between the two
functions of the 5720?

The 5720 is a dual-function device, and this bit selects the clock for
the PCIe transaction layer. The driver already treats parts of the 5720
CPMU block as chip-wide. For example, tg3_set_function_status() keeps
per-function fields in TG3_CPMU_DRV_STATUS, serialized by the APE GPIO
lock:

    status = tr32(TG3_CPMU_DRV_STATUS);

    shift = TG3_APE_GPIO_MSG_SHIFT + 4 * tp->pci_fn;

If 0x3638 is shared, a chip reset of the other port would clear the bit
that this port set in tg3_power_down_prepare(). That reset could come
from tg3_open(), reset_task, an MTU change, an ethtool self-test, or that
port's own tg3_close()->tg3_halt() during shutdown.

As an example, say port 0 has WoL at 1G and port 1 has WoL disabled. Both
are closed during shutdown, and port 1's halt clears the bit. Would port
0 then be back in the failing state?

Each function's read-modify-write also runs under its own tp->lock only,
so concurrent updates from the two functions could lose an update.

If the register is per-function, none of this applies. Could the scope of
this register be documented? If it is shared, could access be coordinated
across functions, as the driver does for other shared power-management
state on this ASIC?

> +
>  	/* Increase the core clock speed to fix tx timeout issue for 5762
>  	 * with 100Mbps link speed.
>  	 */

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108065526.710152-1-pavan.chebbi%40broadcom.com

Re: [PATCH net 2/4] tg3: Select PLL clock when preparing for shutdown

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

>
> [Severity: Medium]
> Does active_speed here describe the link the chip will be using while it
> waits for the magic packet?
>

The check intentionally uses the link speed recorded before power
down, not the speed negotiated for WoL.
The failure was reported and the fix confirmed on a link that was at
1G before power down. I will amend the
commit message.

<-->
> [Severity: Medium]
> Is TG3_CPMU_PLL_CTRL_STAT per-function, or is it shared between the two
> functions of the 5720?
>

Hmm.. yea the register is documented as reset only by POR or CPMU
register software reset.
So I may need to treat is as shared. I will confirm this in the
bootcode and make the required change in the v2

pw-bot: cr

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]

[PATCH net 3/4] tg3: Skip querying for management FW when no NCSI present

[Pavan Chebbi]

On BCM5719/BCM5720, when the NCSI component is missing in the NVRAM,
driver should not assume that "DASH" management FW is running by
default. In this case if user runs ethtool -i command, invalid string
"DASH" and garbage DASH FW version is being displayed. However, the
5719/20 chipset cannot have any "DASH" management firmware running.

Hence skip querying the APE FW version registers and adding the "DASH"
string to the tp->fw_ver if NCSI is not present. Just report the
bootcode version.

This was seen by a user.  With this change, ethtool -i displays only the
bootcode version.

Fixes: ecc796486f0a ("tg3: Detect APE firmware types")
Reviewed-by: Somnath Kotur <somnath.kotur@broadcom.com>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index e9b48e58fdd4..16f8368874e7 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -16029,19 +16029,21 @@ static void tg3_probe_ncsi(struct tg3 *tp)
 
 static void tg3_read_dash_ver(struct tg3 *tp)
 {
-	int vlen;
-	u32 apedata;
 	char *fwtype;
-
-	apedata = tg3_ape_read32(tp, TG3_APE_FW_VERSION);
+	u32 apedata;
+	int vlen;
 
 	if (tg3_flag(tp, APE_HAS_NCSI))
 		fwtype = "NCSI";
 	else if (tp->pdev->device == TG3PCI_DEVICE_TIGON3_5725)
 		fwtype = "SMASH";
+	else if (tg3_asic_rev(tp) == ASIC_REV_5719 ||
+		 tg3_asic_rev(tp) == ASIC_REV_5720)
+		return;
 	else
 		fwtype = "DASH";
 
+	apedata = tg3_ape_read32(tp, TG3_APE_FW_VERSION);
 	vlen = strlen(tp->fw_ver);
 
 	snprintf(&tp->fw_ver[vlen], TG3_VER_SIZE - vlen, " %s v%d.%d.%d.%d",
-- 
2.52.0

[PATCH net 4/4] tg3: Fix heap overflow in unbuffered NVRAM write path

[Pavan Chebbi]

From: Shravya KN <shravya.k-n@broadcom.com>

tg3_nvram_write_block_unbuffered() reads a whole NVRAM page into a
buffer of pagesize bytes, copies the caller's data into it at page_off,
and writes the page back.  The copy size was clamped to pagesize rather
than to the room left in the page, pagesize - page_off, so a write at an
offset that is not page aligned, as ETHTOOL_SEEPROM allows, copies past
the end of the buffer and corrupts the kernel heap.

Clamp size to pagesize - page_off.  Also advance buf by size on each
iteration, as the data of a write that spans pages was otherwise taken
from the start of the caller's buffer for every page.

This was found by an AI code review scan.  It has not been triggered, so
the issue is theoretical.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reviewed-by: Vishvambar Panth S <vishvambar.panth-s@broadcom.com>
Reviewed-by: Michael Chan <michael.chan@broadcom.com>
Signed-off-by: Shravya KN <shravya.k-n@broadcom.com>
Signed-off-by: Pavan Chebbi <pavan.chebbi@broadcom.com>
---
 drivers/net/ethernet/broadcom/tg3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 16f8368874e7..5b5b21d759a6 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -3399,13 +3399,14 @@ static int tg3_nvram_write_block_unbuffered(struct tg3 *tp, u32 offset, u32 len,
 			break;
 
 		page_off = offset & pagemask;
-		size = pagesize;
+		size = pagesize - page_off;
 		if (len < size)
 			size = len;
 
 		len -= size;
 
 		memcpy(tmp + page_off, buf, size);
+		buf += size;
 
 		offset = offset + (pagesize - page_off);
 
-- 
2.52.0

Re: [PATCH net 4/4] tg3: Fix heap overflow in unbuffered NVRAM write path

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] On SST flash, tg3_nvram_write_block_unbuffered() still overflows
  its heap buffer and still corrupts NVRAM on multi-page writes.
- [Medium] The new expression `size = pagesize - page_off` in
  tg3_nvram_write_block_unbuffered() assumes page_off < pagesize.
- [Low] The commit message understates the impact of the second fix.

--

tg3: Fix heap overflow in unbuffered NVRAM write path

This patch changes tg3_nvram_write_block_unbuffered() to clamp the
per-page copy size to pagesize - page_off instead of pagesize. It also
advances buf by size on each iteration, so a write spanning pages takes
the correct data for each page.

> This was found by an AI code review scan.  It has not been triggered, so
> the issue is theoretical.

[Severity: Low]
Is "theoretical" accurate for the buf += size part of this fix?

That word may be fair for the heap overflow. Before this patch, though,
any unbuffered flash write longer than one flash page reprogrammed every
later page with the first page's data from buf. That includes
page-aligned writes.

For writes that were not page aligned, len -= size also used up more
bytes than actually went into the page. The tail of the write was then
dropped without any error.

ethtool_set_eeprom() sends chunks of up to PAGE_SIZE. The unbuffered
parts (ATMEL_AT25F512, SAIFUN) have 256-byte pages, so ordinary
ethtool -E usage would hit this.

Could the description say so, to help stable and distro triage?

> diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
> index 16f8368874e7b..5b5b21d759a63 100644
> --- a/drivers/net/ethernet/broadcom/tg3.c
> +++ b/drivers/net/ethernet/broadcom/tg3.c
> @@ -3399,13 +3399,14 @@ static int tg3_nvram_write_block_unbuffered(struct tg3 *tp, u32 offset, u32 len,
>  			break;
>  
>  		page_off = offset & pagemask;
> -		size = pagesize;
> +		size = pagesize - page_off;

[Severity: High]
Does this fix hold up on SST flash parts?

This clamp, and the pagemask = pagesize - 1 it depends on, both assume
the page size is a power of two. tg3_get_nvram_info() picks a page size
that is not, and it does not set NVRAM_BUFFERED for it:

drivers/net/ethernet/broadcom/tg3.c:tg3_get_nvram_info() {
    ...
		case FLASH_VENDOR_SST_SMALL:
		case FLASH_VENDOR_SST_LARGE:
			tp->nvram_jedecnum = JEDEC_SST;
			tp->nvram_pagesize = SST_25VF0X0_PAGE_SIZE;
			break;
    ...
}

drivers/net/ethernet/broadcom/tg3.h:
#define SST_25VF0X0_PAGE_SIZE		4098

When FLASH is set (from NVRAM_CFG1_FLASHIF_ENAB), tg3_nvram_write_block()
sends writes to tg3_nvram_write_block_unbuffered() with pagesize == 4098.

tmp = kmalloc(4098), but the read-back loop earlier in this function
still runs with j == 4096:

	for (j = 0; j < pagesize; j += 4) {
		ret = tg3_nvram_read_be32(tp, phy_addr + j,
					  (__be32 *) (tmp + j));

Doesn't that store 4 bytes at tmp + 4096, which is 2 bytes past the end
of the allocation? This runs on every call, before the memcpy() this
patch clamps. The write-back loop then reads that same word.

Here pagemask is 0x1001, which is not a real mask. For dword-aligned
offsets, page_off can only be 0 or 0x1000, so this new size is either
4098 or 2.

As an example, take ethtool_set_eeprom() sending its second PAGE_SIZE
chunk at offset 0x1000:

  phy_addr = 0x1000 & ~0x1001 = 0
  page_off = 0x1000
  size     = 2
  offset   = offset + (pagesize - page_off) -> 0x1002, 0x1004, ...

phy_addr then steps through 2, 4, 6, ... while page_off stays at 0x1000.
That is about 2048 iterations. Each one erases and rewrites sector 0 at a
misaligned base and writes only 2 bytes of user data. The function still
returns 0.

In the write loop, j == pagesize - 4 (4094) is never true, so
NVRAM_CMD_LAST is never sent either.

The commit message says both the heap overflow and spanning writes are
fixed. Is either true on this hardware?

SST25VF0x0 sectors are 4 KiB, so 4098 looks like a typo for 4096. Should
SST_25VF0X0_PAGE_SIZE be corrected? The other option is for the
unbuffered path to reject page sizes that are not a power of two or not
a multiple of 4.

[Severity: Medium]
What happens here when tp->nvram_pagesize is 0?

On 5750/5780-class chips, tg3_get_nvram_info() sets FLASH from
NVRAM_CFG1_FLASHIF_ENAB separately from the vendor switch, and that
switch has no default case. For a vendor encoding that is not handled,
pagesize stays 0 and NVRAM_BUFFERED stays clear, so writes take this
path.

In that state:

  tmp      = kmalloc(0)          -> ZERO_SIZE_PTR, passes the NULL check
  pagemask = 0 - 1               -> 0xffffffff
  page_off = offset & pagemask   -> offset
  size     = 0 - offset          -> wraps to a large value, clamped to len

For any non-zero offset, the memcpy() below then becomes
memcpy(ZERO_SIZE_PTR + offset, buf, len).

Before this patch, size stayed 0 in this state. The loop never ended and
kept erasing NVRAM address 0, but nothing was written to memory.

Could this change turn that hang into a wild kernel write, done while
rtnl_lock and the NVRAM lock are held?

It isn't clear whether real hardware ever reports such an encoding. A
pagesize check at the top of the function (non-zero, power of two,
multiple of 4) would cover this case and the SST 4098 case above.

>  		if (len < size)
>  			size = len;
>  
>  		len -= size;
>  
>  		memcpy(tmp + page_off, buf, size);
> +		buf += size;
>  
>  		offset = offset + (pagesize - page_off);
>

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261108065526.710152-1-pavan.chebbi%40broadcom.com

Re: [PATCH net 4/4] tg3: Fix heap overflow in unbuffered NVRAM write path

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

> [Severity: Low]
> Is "theoretical" accurate for the buf += size part of this fix?
>

Agreed.  The buf += size part is not theoretical, it is reachable with
ethtool -E on unbuffered flash.
We will amend the commit.

<-->
> [Severity: High]
> Does this fix hold up on SST flash parts?
>

That is a separate problem from the one this patch fixes.  It has been
there since the original code, and this patch does not change anything
regarding that part.
The heap overflow this patch fixes is the memcpy() of pagesize bytes
at a non-zero page_off, which applies to every unbuffered part.

<-->
> [Severity: Medium]
> What happens here when tp->nvram_pagesize is 0?
>

Agreed. We will reject a zero page size with -EINVAL before allocating
the buffer, in the v2

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5469 bytes --]