[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

[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

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.