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

[netdev-bot+sashiko@kernel.org]

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