Re: [PATCH v2] usb: rtl8150: avoid using uninitialized CSCR value

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
usb: rtl8150: avoid using uninitialized CSCR value

This commit fixes an uninitialized variable bug in set_carrier() where
get_registers() could fail, leaving tmp uninitialized. The fix adds a
return value check to bail out on error, preserving the existing carrier
state.

Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")

> set_carrier() reads CSCR via get_registers() (a USB control transfer)
> without checking the return value, so a transient control transfer
> failure would leave the on-stack "tmp" uninitialized and then be used
> to decide the netif carrier state.
>
> Check the return value of get_registers() and bail out on error,
> leaving the previously known carrier state untouched. A failed USB
> control transfer here may be transient (e.g. EMI, flaky cable, retries
> exhausted), so it is wrong to force the link down on such failures and
> cause the carrier state to toggle unnecessarily.

The commit message says "it is wrong to force the link down on such
failures" - but is this characterization accurate?

The original (pre-patch) code did not deliberately force the link down on
failure. It used an uninitialized value, which would randomly either set
carrier on or off depending on stack contents. The statement reads as if
the original code forced link down, but the original bug was random
behavior from uninitialized data.

The "force the link down" language more accurately describes what the
rejected v1 patch did, and why v2 changed the approach to just returning.

>
> This only addresses the uninitialized-value usage and does not change
> link-state policy.

Is this description fully accurate? The patch does change observable
behavior:

Before: on get_registers() failure, carrier state was set to a random value
(on or off based on stack contents).

After: on failure, carrier state is preserved unchanged.

This is a change from "set carrier based on garbage data" to "preserve
previous carrier state." While "using uninitialized data" was never an
intentional policy, the commit does introduce a new explicit behavior
(early return preserving state) that did not exist before.

Additionally, set_carrier() is only called from rtl8150_open() in this
driver, meaning the "previously known carrier state" at that point is the
default state from alloc_etherdev (carrier ON, since __LINK_STATE_NOCARRIER
bit is 0 in zero-initialized memory). On first open, a get_registers()
failure would preserve carrier=ON, which could report link up when the
register read failed.

> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..76962dcd127d 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -722,7 +722,8 @@ static void set_carrier(struct net_device *netdev)
>  	rtl8150_t *dev = netdev_priv(netdev);
>  	short tmp;
>
> -	get_registers(dev, CSCR, 2, &tmp);
> +	if (get_registers(dev, CSCR, 2, &tmp) < 0)
> +		return;
>  	if (tmp & CSCR_LINK_STATUS)
>  		netif_carrier_on(netdev);
>  	else
-- 
pw-bot: cr