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

[Manuel Diewald]

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.

-- 
 Manuel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20241025/4171af2a/attachment.sig>