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

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

[Morduan Zang]

Check get_registers() when reading CSCR in set_carrier().
If the control transfer fails, don't use the stack value.

Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
---
 drivers/net/usb/rtl8150.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 4cda0643afb6..7e32726d3e6f 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -722,7 +722,11 @@ 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) {
+		netif_carrier_off(netdev);
+		return;
+	}
+
 	if (tmp & CSCR_LINK_STATUS)
 		netif_carrier_on(netdev);
 	else
-- 
2.50.1

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

[Petko Manolov]

On 26-04-02 15:07:43, Morduan Zang wrote:
> Check get_registers() when reading CSCR in set_carrier().
> If the control transfer fails, don't use the stack value.
> 
> Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
> ---
>  drivers/net/usb/rtl8150.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..7e32726d3e6f 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -722,7 +722,11 @@ 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) {
> +		netif_carrier_off(netdev);
> +		return;
> +	}
> +
>  	if (tmp & CSCR_LINK_STATUS)
>  		netif_carrier_on(netdev);
>  	else
> -- 

Nice catch.  You can add my Acked-by.


cheers,
Petko

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

[Simon Horman]

On Thu, Apr 02, 2026 at 03:07:43PM +0800, Morduan Zang wrote:
> Check get_registers() when reading CSCR in set_carrier().
> If the control transfer fails, don't use the stack value.
> 
> Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
> ---
>  drivers/net/usb/rtl8150.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..7e32726d3e6f 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -722,7 +722,11 @@ 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) {
> +		netif_carrier_off(netdev);
> +		return;
> +	}
> +
>  	if (tmp & CSCR_LINK_STATUS)
>  		netif_carrier_on(netdev);
>  	else

I wonder if we can handle this a bit more succinctly,
while still making it clear that the error is handled.
Something like this:

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 4cda0643afb6..816759ced56c 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)
+		tmp = 0;
 	if (tmp & CSCR_LINK_STATUS)
 		netif_carrier_on(netdev);
 	else

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

[Petko Manolov]

On 26-04-03 16:45:38, Simon Horman wrote:
> On Thu, Apr 02, 2026 at 03:07:43PM +0800, Morduan Zang wrote:
> > Check get_registers() when reading CSCR in set_carrier().
> > If the control transfer fails, don't use the stack value.
> > 
> > Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
> > ---
> >  drivers/net/usb/rtl8150.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> > index 4cda0643afb6..7e32726d3e6f 100644
> > --- a/drivers/net/usb/rtl8150.c
> > +++ b/drivers/net/usb/rtl8150.c
> > @@ -722,7 +722,11 @@ 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) {
> > +		netif_carrier_off(netdev);
> > +		return;
> > +	}
> > +
> >  	if (tmp & CSCR_LINK_STATUS)
> >  		netif_carrier_on(netdev);
> >  	else
> 
> I wonder if we can handle this a bit more succinctly,
> while still making it clear that the error is handled.
> Something like this:
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..816759ced56c 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)
> +		tmp = 0;
>  	if (tmp & CSCR_LINK_STATUS)
>  		netif_carrier_on(netdev);
>  	else

I was wondering if calling netif_carrier_off() is the right thing to do in case
get_registers() fail.

There are multiple get_registers() calls that don't check the error and if we do
this in set_carrier() maybe we should do the same thing across the whole driver?


cheers,
Petko

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

[Andrew Lunn]

> > -	get_registers(dev, CSCR, 2, &tmp);
> > +	if (get_registers(dev, CSCR, 2, &tmp) < 0)
> > +		tmp = 0;
> >  	if (tmp & CSCR_LINK_STATUS)
> >  		netif_carrier_on(netdev);
> >  	else
> 
> I was wondering if calling netif_carrier_off() is the right thing to do in case
> get_registers() fail.
> 
> There are multiple get_registers() calls that don't check the error and if we do
> this in set_carrier() maybe we should do the same thing across the whole driver?

What does it actually mean if get_registers() fails? The device is
gone? Hot unplugged? If so, you are going to get a cascade of errors,
and then hopefully the USB core code removes the device?

Are there any legitimate reasons for get_registers() to fail if the
device is still plugged in?

It seems netif_carrier_off() is unnecessary?

   Andrew

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

[Morduan Zang]

> I wonder if we can handle this a bit more succinctly,
> while still making it clear that the error is handled.
> Something like this:
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 4cda0643afb6..816759ced56c 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)
> + tmp = 0;
>   if (tmp & CSCR_LINK_STATUS)
>   netif_carrier_on(netdev);
>   else
Thanks for your suggestion.
I agree that errors should be handled explicitly, but I prefer not to force the carrier off
when CSCR read fails. The control transfer error here may be transient, and treating it
as a link break would cause the carrier state to toggle frequently.
Therefore, if the get_registers() call fails, should we keep the previous carrier state and
return directly? (Perhaps with a rate-limited warning as an option), while still avoiding
the use of uninitialized stack values.

If agreed, I will send a v2 incorporating the suggestions. 
 

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

[Michal Pecio]

On Mon, 6 Apr 2026 01:38:06 +0200, Andrew Lunn wrote:
> > > -	get_registers(dev, CSCR, 2, &tmp);
> > > +	if (get_registers(dev, CSCR, 2, &tmp) < 0)
> > > +		tmp = 0;
> > >  	if (tmp & CSCR_LINK_STATUS)
> > >  		netif_carrier_on(netdev);
> > >  	else  
> > 
> > I was wondering if calling netif_carrier_off() is the right thing
> > to do in case get_registers() fail.
> > 
> > There are multiple get_registers() calls that don't check the error
> > and if we do this in set_carrier() maybe we should do the same
> > thing across the whole driver?  
> 
> What does it actually mean if get_registers() fails? The device is
> gone? Hot unplugged? If so, you are going to get a cascade of errors,
> and then hopefully the USB core code removes the device?
> 
> Are there any legitimate reasons for get_registers() to fail if the
> device is still plugged in?

In principle it might be temporary EMI or a flaky cable. These errors
rarely reach drivers due to retries in USB layer, but in extreme cases
the device may be in unknown state and it may become functional later.
IIRC net layer has some operations which are presumed trivial enough
that they would never fail, so this could be annoying.

It does seem rare enough in practice that for 25 years nobody noticed
carrier status being set to a random vaule by this driver.

BTW, some functions like rtl8150_reset() pre-set data to a value which
will be safe in case of get_register() failure. But here, unhandled
set_register() error is dodgy - the 0x10 bit may never turn on.

Regards,
Michal

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

[Andrew Lunn]

> BTW, some functions like rtl8150_reset() pre-set data to a value which
> will be safe in case of get_register() failure. But here, unhandled
> set_register() error is dodgy - the 0x10 bit may never turn on.

rtl8150_reset() is not great, it would be better to use something from
iopoll.h, and return ETIMEDOUT, but it does at least have a
timeout. And it looks like all other such loops have some form of
timeout.

Could this driver use some love? Yes. Does it mean not checking for
get_register() failure makes the driver badly broken? Probably not.

	Andrew

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

[Morduan Zang]

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.

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

Reported-by: syzbot+9db6c624635564ad813c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=9db6c624635564ad813c
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: Petko Manolov <petkan@nucleusys.com>
Cc: Simon Horman <horms@kernel.org>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Michal Pecio <michal.pecio@gmail.com>
Signed-off-by: Morduan Zang <zhangdandan@uniontech.com>
---
Changes in v2:
 - Do not force netif_carrier_off() on get_registers() failure; instead
   return and keep the previous carrier state. A transient USB control
   transfer failure should not cause carrier to toggle.
   (based on review feedback from Simon Horman, Petko Manolov,
    Andrew Lunn and Michal Pecio.)
 - Minimal change: only avoid the uninitialized read; no link-state
   policy change. Petko's Ack on v1 is not carried over because v2
   changes the error-handling behavior.

Changes in v1:
 - Initial submission: on get_registers() failure call
   netif_carrier_off() and return.
---
 drivers/net/usb/rtl8150.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 4cda0643afb6..816759ced56c 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
-- 
2.50.1

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

[Jakub Kicinski]

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