[PATCH net 0/2] net: usb: lan78xx: do not use uninitialized data after short reads

[PATCH net 0/2] net: usb: lan78xx: do not use uninitialized data after short reads

[Wei Jie Law]

lan78xx_read_reg() and lan78xx_read_stats() accept a control transfer
that returns fewer bytes than requested and then use the uninitialized
rest of their kmalloc()ed buffers. A device that sends short replies can
get stale heap contents written back to it as the MAC address, and makes
ethtool -S report them to unprivileged users, including kernel text and
direct map pointers, on systems with init_on_alloc=0.

Patch 2 makes both helpers fail on short reads. Patch 1 comes first so
that a failing statistics read does not report uninitialized stack data
instead.

The bugs need a malicious or emulated USB device, which
Documentation/process/threat-model.rst treats as a regular bug rather
than a vulnerability, so they are posted publicly.

Testing: the series builds without warnings (W=1) on net and passes
checkpatch --strict. It applies unchanged to v6.12.105, where it was
tested with a LAN7800 emulated by raw-gadget + dummy_hcd, booted with
init_on_alloc=0:

- Zero-length statistics replies. Before: 40 to 88 of the 235 ethtool -S
  counters are non-zero, including usb_api_blocking_completion() and
  direct map pointers. After: every read fails with -EIO and all
  counters stay zero.

- Zero-length RX_ADDRL/RX_ADDRH replies. Before: the MAC address
  differs on every probe and is written back to the device. After: the
  probe fails with -EIO.

Both bugs were found, and the patches written, with LLM assistance,
hence the Assisted-by tags.

Wei Jie Law (2):
  net: usb: lan78xx: do not update statistics after a failed read
  net: usb: lan78xx: reject short register and statistics reads

 drivers/net/usb/lan78xx.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)


base-commit: af32da41b0327b9c6a37856ba82b6760d6c8d10e
-- 
2.43.0

[PATCH net 1/2] net: usb: lan78xx: do not update statistics after a failed read

[Wei Jie Law]

lan78xx_update_stats() reads the hardware counters into an on-stack
struct lan78xx_statstage. If lan78xx_read_stats() fails, only the
rollover check is skipped: the counters are still recomputed from the
uninitialized stack buffer and stored in dev->stats.curr_stat, which is
then reported to user space by ethtool -S.

Keep the previous values when the read fails.

Found by code review with LLM assistance. Tested together with the next
patch, which makes short reads fail: with an emulated device, ethtool -S
then keeps reporting the previous counters.

Fixes: 20ff55655a93 ("lan78xx: handle statistics counter rollover")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Wei Jie Law <law.wei.jie@gt.tech.gov.sg>
---
 drivers/net/usb/lan78xx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5655941f1478..cb8204371f5c 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -789,11 +789,13 @@ static void lan78xx_update_stats(struct lan78xx_net *dev)
 
 	mutex_lock(&dev->stats.access_lock);
 
-	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0)
+	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0) {
 		lan78xx_check_stat_rollover(dev, &lan78xx_stats);
 
-	for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
-		data[i] = (u64)p[i] + ((u64)count[i] * ((u64)max[i] + 1));
+		for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
+			data[i] = (u64)p[i] +
+				  ((u64)count[i] * ((u64)max[i] + 1));
+	}
 
 	mutex_unlock(&dev->stats.access_lock);
 
-- 
2.43.0

[PATCH net 2/2] net: usb: lan78xx: reject short register and statistics reads

[Wei Jie Law]

lan78xx_read_reg() and lan78xx_read_stats() read from the device into
a freshly kmalloc()ed buffer, but only treat a negative return value
from usb_control_msg() as an error. A device that answers with fewer
bytes than requested leaves part or all of the buffer uninitialized,
and those bytes are then used as the register value or the counters.

With an emulated device on a kernel booted with init_on_alloc=0:

- A zero-length reply to the statistics request makes ethtool -S, which
  needs no privileges, report the stale contents of the kmalloc-192
  buffer. One counter pair read back as 0xffffffff8a427800, the
  address of usb_api_blocking_completion(), next to several direct
  map pointers.

- A zero-length reply to the RX_ADDRL/RX_ADDRH reads in
  lan78xx_init_mac_address() turns uninitialized heap memory into the
  MAC address (12:85:1f:dc:2e:bd and e6:93:44:41:9b:9f on two probes),
  which is then written back to the device in RX_ADDRL/RX_ADDRH and
  MAF_LO(0)/MAF_HI(0).

Treat a short transfer as -EIO in both helpers. With this change the
same device fails to probe with -EIO, and with only the statistics
reply cut short all counters stay zero.

Found by code review with LLM assistance and reproduced with an
emulated USB device (raw-gadget + dummy_hcd).

Fixes: 55d7de9de6c3 ("Microchip's LAN7800 family USB 2/3 to 10/100/1000 Ethernet device driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Wei Jie Law <law.wei.jie@gt.tech.gov.sg>
---
 drivers/net/usb/lan78xx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index cb8204371f5c..e6be066349de 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -612,6 +612,9 @@ static int lan78xx_read_reg(struct lan78xx_net *dev, u32 index, u32 *data)
 			      USB_VENDOR_REQUEST_READ_REGISTER,
 			      USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
 			      0, index, buf, 4, USB_CTRL_GET_TIMEOUT);
+	if (ret >= 0 && ret != sizeof(*buf))
+		ret = -EIO;
+
 	if (likely(ret >= 0)) {
 		le32_to_cpus(buf);
 		*data = *buf;
@@ -695,6 +698,9 @@ static int lan78xx_read_stats(struct lan78xx_net *dev,
 			      (void *)stats,
 			      sizeof(*stats),
 			      USB_CTRL_SET_TIMEOUT);
+	if (ret >= 0 && ret != sizeof(*stats))
+		ret = -EIO;
+
 	if (likely(ret >= 0)) {
 		src = (u32 *)stats;
 		dst = (u32 *)data;
-- 
2.43.0

Re: [PATCH net 2/2] net: usb: lan78xx: reject short register and statistics reads

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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] Three statements in this commit message (tagged for stable) do
  not match the code at its parent.

--

net: usb: lan78xx: reject short register and statistics reads

This patch makes lan78xx_read_reg() and lan78xx_read_stats() return -EIO
when usb_control_msg() transfers fewer bytes than requested. Before this,
uninitialized kmalloc() memory could be used as a register value or as the
statistics counters.

> With an emulated device on a kernel booted with init_on_alloc=0:
>
> - A zero-length reply to the statistics request makes ethtool -S, which
>   needs no privileges, report the stale contents of the kmalloc-192
>   buffer. One counter pair read back as 0xffffffff8a427800, the
>   address of usb_api_blocking_completion(), next to several direct
>   map pointers.
>
> - A zero-length reply to the RX_ADDRL/RX_ADDRH reads in
>   lan78xx_init_mac_address() turns uninitialized heap memory into the
>   MAC address (12:85:1f:dc:2e:bd and e6:93:44:41:9b:9f on two probes),
>   which is then written back to the device in RX_ADDRL/RX_ADDRH and
>   MAF_LO(0)/MAF_HI(0).
>
> Treat a short transfer as -EIO in both helpers. With this change the
> same device fails to probe with -EIO, and with only the statistics
> reply cut short all counters stay zero.

[Severity: Medium]

Can the zero-length statistics case still be reproduced at this patch's
parent?

The previous patch in the series, "net: usb: lan78xx: do not update
statistics after a failed read", already updates curr_stat only when the
return value is positive:

drivers/net/usb/lan78xx.c:lan78xx_update_stats() {
    ...
	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0) {
		lan78xx_check_stat_rollover(dev, &lan78xx_stats);

		for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
			data[i] = (u64)p[i] +
				  ((u64)count[i] * ((u64)max[i] + 1));
	}
    ...
}

A zero-length reply makes usb_control_msg() return 0. Then
lan78xx_read_stats() returns 0 and dev->stats.curr_stat is never written.

With that patch applied, it looks like only a partial reply of 1 to 187
bytes still leaks the uninitialized tail of the kmalloc() buffer. The
message doesn't mention that case.

For the same reason, wouldn't "with only the statistics reply cut short all
counters stay zero" also be true without this patch when the reply has zero
length?

Could the message also mention that it depends on the previous patch? Both
patches are Cc: stable, but they have different Fixes: tags (20ff55655a93
and 55d7de9de6c3).

If this patch is backported alone, lan78xx_read_stats() returns -EIO
without writing data. The old lan78xx_update_stats() still runs the loop
unconditionally:

	if (lan78xx_read_stats(dev, &lan78xx_stats) > 0)
		lan78xx_check_stat_rollover(dev, &lan78xx_stats);

	for (i = 0; i < (sizeof(lan78xx_stats) / (sizeof(u32))); i++)
		data[i] = (u64)p[i] + ((u64)count[i] * ((u64)max[i] + 1));

The heap leak would then become a leak of the uninitialized on-stack
lan78xx_stats through ethtool -S, and the counters would not stay zero.

In the MAC paragraph, is the RX_ADDRL/RX_ADDRH write-back accurate? In
lan78xx_init_mac_address(), those registers are only written when the
address is invalid, after addr has been replaced from DT, EEPROM, OTP or a
random address:

	if (!is_valid_ether_addr(addr)) {
		...
		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);

Both quoted addresses (12:85:1f:dc:2e:bd and e6:93:44:41:9b:9f) are valid
unicast addresses, so they skip that block. Only MAF_LO(0)/MAF_HI(0) and
the netdev address set by eth_hw_addr_set() receive the value taken from
the heap.

The code change itself looks correct.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014025602.26334-1-law.wei.jie%40gt.tech.gov.sg

Re: [PATCH net 2/2] net: usb: lan78xx: reject short register and statistics reads

[Wei Jie Law]

Sashiko is correct. Will be submitting v2 with commit messages changed:
- with patch 1 a zero-length stats reply is already harmless; the remaining
  leak is a 1 to 187 byte reply (the counter block is 188 bytes). The message
  now says so and no longer claims the counters "stay zero" because of 2/2.
- it says that 2/2 depends on 1/2 (otherwise the leak moves to the stack) and
  that both should be backported together.
- RX_ADDRL/RX_ADDRH are only written back for an invalid address; the heap
  derived address is written to MAF_LO(0)/MAF_HI(0) and set as the netdev address.

pw-bot: cr