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

[Andrew Lunn]

On Sun, Oct 18, 2026 at 02:17:34PM +0800, Wei Jie Law wrote:
> From: Wei Jie Law <98lawweijie@xxxxxxxxx>
> 
> 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.

You are assuming reading registers can fail with sort reads. To be
symmetric, don't you have to assume writes can also fail in the same
way? Should lan78xx_write_reg() have a similar test?

> With an emulated device on a kernel booted with init_on_alloc=0:
> 
> - A reply to the statistics request that is shorter than the 188 byte
>   counter block leaves the rest of the kmalloc-192 buffer stale, and
>   ethtool -S, which needs no privileges, reports it. Before the
>   previous patch, a zero-length reply was enough: one counter pair read
>   back as 0xffffffff8a427800, the address of
>   usb_api_blocking_completion(), next to several direct map pointers.
>   With the previous patch a zero-length reply is already harmless, but
>   a reply of 1 to 187 bytes still gets through.
> 
> - 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).
>   Both of those are valid unicast addresses, so they are used as they
>   are: written to MAF_LO(0)/MAF_HI(0) and set as the address of the
>   net device, where anyone can read it.

If short reads can happen, bad things are expected to happen. No need
for examples.

> This patch needs the previous one. Without it, a failed statistics read
> would make lan78xx_update_stats() compute the counters from its
> uninitialized on-stack buffer, so the leak would move from the heap to
> the stack. Both patches are Cc: stable and should be backported
> together.

This belongs below the --- so that is it not part of the commit
message.

    Andrew

---
pw-bot: cr