LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Jakub Kicinski <kuba@kernel.org>
To: Pavel Skripkin <paskripkin@gmail.com>
Cc: davem@davemloft.net, linux@rempel-privat.de,
	himadrispandya@gmail.com, andrew@lunn.ch,
	linux-usb@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com,
	Robert Foss <robert.foss@collabora.com>,
	Vincent Palatin <vpalatin@chromium.org>
Subject: Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read
Date: Fri, 13 Aug 2021 15:52:26 -0700	[thread overview]
Message-ID: <20210813155226.651c74f0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20210813224219.11359-1-paskripkin@gmail.com>

On Sat, 14 Aug 2021 01:42:19 +0300 Pavel Skripkin wrote:
> 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 cheking and just continue the loop instead of
> checking uninit value.
> 
> Fixes: 8a46f665833a ("net: asix: Avoid looping when the device is disconnected")

This is not the right tag, the Fixes tag should point to the commit
where the problem is introduced. Robert/Vincent only added some error
checking, the issue was there before, right?

Once you locate the right starting point for the fix please make sure
to add the author to CC.

Thanks!

> Reported-by: syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>

>  drivers/net/usb/asix_common.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/asix_common.c b/drivers/net/usb/asix_common.c
> index ac92bc52a85e..7019c25e591c 100644
> --- a/drivers/net/usb/asix_common.c
> +++ b/drivers/net/usb/asix_common.c
> @@ -468,18 +468,25 @@ int asix_mdio_read(struct net_device *netdev, int phy_id, int loc)
>  	struct usbnet *dev = netdev_priv(netdev);
>  	__le16 res;
>  	u8 smsr;
> -	int i = 0;
> +	int i;
>  	int ret;

nit: move i after ret, reverse xmas tree style

>  	mutex_lock(&dev->phy_mutex);
> -	do {
> +	for (i = 0; i < 30; ++i) {
>  		ret = asix_set_sw_mii(dev, 0);
>  		if (ret == -ENODEV || ret == -ETIMEDOUT)
>  			break;
>  		usleep_range(1000, 1100);
>  		ret = asix_read_cmd(dev, AX_CMD_STATMNGSTS_REG,
>  				    0, 0, 1, &smsr, 0);
> -	} while (!(smsr & AX_HOST_EN) && (i++ < 30) && (ret != -ENODEV));
> +		if (ret == -ENODEV)
> +			break;
> +		else if (ret < 0)
> +			continue;
> +		else if (smsr & AX_HOST_EN)
> +			break;
> +	}
> +
>  	if (ret == -ENODEV || ret == -ETIMEDOUT) {
>  		mutex_unlock(&dev->phy_mutex);
>  		return ret;

Code LGTM, do other functions which Robert/Vincent touched not need the
same treatment tho?

  reply	other threads:[~2021-08-13 22:52 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 16:01 [PATCH] " Pavel Skripkin
2021-08-13 22:23 ` Andrew Lunn
2021-08-13 22:29   ` Pavel Skripkin
2021-08-13 22:42   ` [PATCH v2] " Pavel Skripkin
2021-08-13 22:52     ` Jakub Kicinski [this message]
2021-08-14 13:55       ` [PATCH v3] net: asix: fix uninit value bugs Pavel Skripkin
2021-08-14 15:36         ` Andrew Lunn
2021-08-14 15:40           ` Pavel Skripkin
2021-08-14 16:20           ` Pavel Skripkin
2021-08-17 16:37           ` [PATCH v4] " Pavel Skripkin
2021-08-18 10:50             ` patchwork-bot+netdevbpf
2021-08-13 22:57     ` [PATCH v2] net: asix: fix uninit value in asix_mdio_read Andrew Lunn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20210813155226.651c74f0@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
    --to=kuba@kernel.org \
    --cc=andrew@lunn.ch \
    --cc=davem@davemloft.net \
    --cc=himadrispandya@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=linux@rempel-privat.de \
    --cc=netdev@vger.kernel.org \
    --cc=paskripkin@gmail.com \
    --cc=robert.foss@collabora.com \
    --cc=syzbot+a631ec9e717fb0423053@syzkaller.appspotmail.com \
    --cc=vpalatin@chromium.org \
    --subject='Re: [PATCH v2] net: asix: fix uninit value in asix_mdio_read' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).