NACK/Cmnt: [SRU][F][PATCH 1/2] net: asix: fix uninit value bugs

[Koichiro Den]

On Fri, Oct 25, 2024 at 03:49:53PM +0200, Manuel Diewald wrote:
> On Fri, Oct 25, 2024 at 10:27:14PM +0900, Koichiro Den wrote:
> > From: Pavel Skripkin <paskripkin at gmail.com>
> > 
> > Syzbot reported uninit-value in asix_mdio_read(). The problem was in
> > missing error handling. asix_read_cmd() should initialize passed stack
> > variable smsr, but it can fail in some cases. Then while condidition
> > checks possibly uninit smsr variable.
> > 
> > Since smsr is uninitialized stack variable, driver can misbehave,
> > because smsr will be random in case of asix_read_cmd() failure.
> > Fix it by adding error handling and just continue the loop instead of
> > checking uninit value.
> > 
> > Added helper function for checking Host_En bit, since wrong loop was used
> > in 4 functions and there is no need in copy-pasting code parts.
> > 
> > Cc: Robert Foss <robert.foss at collabora.com>
> > Fixes: d9fe64e51114 ("net: asix: Add in_pm parameter")
> > Reported-by: syzbot+a631ec9e717fb0423053 at syzkaller.appspotmail.com
> > Signed-off-by: Pavel Skripkin <paskripkin at gmail.com>
> > Signed-off-by: David S. Miller <davem at davemloft.net>
> > (backported from commit a786e3195d6af183033e86f0518ffd2c51c0e8ac)
> > [koichiroden: Adjusted context due to missing commit d275afb66371 ("net:
> > usb: asix: add error handling for asix_mdio_* functions"), which in turn
> > depends on e532a096be0e ("net: usb: asix: ax88772: add phylib support").
> > Ref. [PATCH net-next v2 0/8] port asix ax88772 to the PHYlib
> > https://lore.kernel.org/linux-usb/20210607082727.26045-1-o.rempel@pengutronix.de/]
> > CVE-2021-47101
> > Signed-off-by: Koichiro Den <koichiro.den at canonical.com>
> > ---
> >  drivers/net/usb/asix_common.c | 71 +++++++++++++++--------------------
> >  1 file changed, 31 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> > index 7bc6e8f856fe..12ce52600eaf 100644
> > --- a/drivers/net/usb/asix_common.c
> > +++ b/drivers/net/usb/asix_common.c
> > @@ -63,6 +63,29 @@ void asix_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> >  			       value, index, data, size);
> >  }
> >  
> > +static int asix_check_host_enable(struct usbnet *dev, int in_pm)
> > +{
> > +	int i, ret;
> > +	u8 smsr;
> > +
> > +	for (i = 0; i < 30; ++i) {
> > +		ret = asix_set_sw_mii(dev, in_pm);
> > +		if (ret == -ENODEV || ret == -ETIMEDOUT)
> > +			break;
> > +		usleep_range(1000, 1100);
> > +		ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
> > +				    0, 0, 1, &smsr, in_pm);
> > +		if (ret == -ENODEV)
> > +			break;
> > +		else if (ret < 0)
> > +			continue;
> > +		else if (smsr & AX_HOST_EN)
> > +			break;
> > +	}
> > +
> > +	return ret;
> 
> There is a follow-up fix available upstream that fixes the return value
> of this new function:
> 
> d1652b70d07c asix: fix wrong return value in asix_check_host_enable()
> 
> I think we should include it in this patchset.

Thank you for catching this. I'll send v2 soon.

> 
> -- 
>  Manuel