| Message ID | 20250319112156.48312-2-qasdev00@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | net: fix bugs and error handling in qinheng ch9200 driver and mii interface | expand |
On Wed, Mar 19, 2025 at 11:21:53AM +0000, Qasim Ijaz wrote: > In mii_nway_restart() during the line: > > bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); > > The code attempts to call mii->mdio_read which is ch9200_mdio_read(). > > ch9200_mdio_read() utilises a local buffer, which is initialised > with control_read(): > > unsigned char buff[2]; > > However buff is conditionally initialised inside control_read(): > > if (err == size) { > memcpy(data, buf, size); > } > > If the condition of "err == size" is not met, then buff remains > uninitialised. Once this happens the uninitialised buff is accessed > and returned during ch9200_mdio_read(): > > return (buff[0] | buff[1] << 8); > > The problem stems from the fact that ch9200_mdio_read() ignores the > return value of control_read(), leading to uinit-access of buff. > > To fix this we should check the return value of control_read() > and return early on error. > > Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com> > Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9 > Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com> > Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") > Cc: stable@vger.kernel.org > Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> Reviewed-by: Simon Horman <horms@kernel.org>
On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote: > --- a/drivers/net/mii.c > +++ b/drivers/net/mii.c > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii) > > /* if autoneg is off, it's an error */ > bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); > + if (bmcr < 0) > + return bmcr; > > if (bmcr & BMCR_ANENABLE) { > bmcr |= BMCR_ANRESTART; We error check just one mdio_read() but there's a whole bunch of them in this file. What's the expected behavior then? Are all of them buggy? This patch should be split into core and driver parts.
On Tue, Mar 25, 2025 at 06:33:07AM -0700, Jakub Kicinski wrote: > On Wed, 19 Mar 2025 11:21:53 +0000 Qasim Ijaz wrote: > > --- a/drivers/net/mii.c > > +++ b/drivers/net/mii.c > > @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii) > > > > /* if autoneg is off, it's an error */ > > bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); > > + if (bmcr < 0) > > + return bmcr; > > > > if (bmcr & BMCR_ANENABLE) { > > bmcr |= BMCR_ANRESTART; > > We error check just one mdio_read() but there's a whole bunch of them > in this file. What's the expected behavior then? Are all of them buggy? > Hi Jakub Apologies for my delayed response, I had another look at this and I think my patch may be off a bit. You are correct that there are multiple mdio_read() calls and looking at the mii.c file we can see that calls to functions like mdio_read (and a lot of others) dont check return values. So in light of this I think a better patch would be to not edit the mii.c file at all and just make ch9200_mdio_read return 0 on error. This way if mdio_read fails and 0 is returned, the check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart will just return 0 and end. If we return a negative on error it may contain the exact bit the function checks. Similiar to this patch: <https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38> If this sounds good, should i send another patch series with all the changes? > This patch should be split into core and driver parts. > -- > pw-bot: cr
On Thu, 10 Apr 2025 23:15:23 +0100 Qasim Ijaz wrote: > Apologies for my delayed response, I had another look at this and I > think my patch may be off a bit. You are correct that there are multiple > mdio_read() calls and looking at the mii.c file we can see that calls to > functions like mdio_read (and a lot of others) dont check return values. > > So in light of this I think a better patch would be to not edit the > mii.c file at all and just make ch9200_mdio_read return 0 on > error. This way if mdio_read fails and 0 is returned, the > check for "bmcr & BMCR_ANENABLE" won't be triggered and mii_nway_restart > will just return 0 and end. If we return a negative on error it may > contain the exact bit the function checks. > > Similiar to this patch: > <https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git/commit/?id=c68b2c9eba38> > > If this sounds good, should i send another patch series with all the > changes? SG
diff --git a/drivers/net/mii.c b/drivers/net/mii.c index 37bc3131d31a..e305bf0f1d04 100644 --- a/drivers/net/mii.c +++ b/drivers/net/mii.c @@ -464,6 +464,8 @@ int mii_nway_restart (struct mii_if_info *mii) /* if autoneg is off, it's an error */ bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); + if (bmcr < 0) + return bmcr; if (bmcr & BMCR_ANENABLE) { bmcr |= BMCR_ANRESTART; diff --git a/drivers/net/usb/ch9200.c b/drivers/net/usb/ch9200.c index f69d9b902da0..a206ffa76f1b 100644 --- a/drivers/net/usb/ch9200.c +++ b/drivers/net/usb/ch9200.c @@ -178,6 +178,7 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc) { struct usbnet *dev = netdev_priv(netdev); unsigned char buff[2]; + int ret; netdev_dbg(netdev, "%s phy_id:%02x loc:%02x\n", __func__, phy_id, loc); @@ -185,8 +186,10 @@ static int ch9200_mdio_read(struct net_device *netdev, int phy_id, int loc) if (phy_id != 0) return -ENODEV; - control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02, - CONTROL_TIMEOUT_MS); + ret = control_read(dev, REQUEST_READ, 0, loc * 2, buff, 0x02, + CONTROL_TIMEOUT_MS); + if (ret < 0) + return ret; return (buff[0] | buff[1] << 8); }
In mii_nway_restart() during the line: bmcr = mii->mdio_read(mii->dev, mii->phy_id, MII_BMCR); The code attempts to call mii->mdio_read which is ch9200_mdio_read(). ch9200_mdio_read() utilises a local buffer, which is initialised with control_read(): unsigned char buff[2]; However buff is conditionally initialised inside control_read(): if (err == size) { memcpy(data, buf, size); } If the condition of "err == size" is not met, then buff remains uninitialised. Once this happens the uninitialised buff is accessed and returned during ch9200_mdio_read(): return (buff[0] | buff[1] << 8); The problem stems from the fact that ch9200_mdio_read() ignores the return value of control_read(), leading to uinit-access of buff. To fix this we should check the return value of control_read() and return early on error. Reported-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com> Closes: https://syzkaller.appspot.com/bug?extid=3361c2d6f78a3e0892f9 Tested-by: syzbot <syzbot+3361c2d6f78a3e0892f9@syzkaller.appspotmail.com> Fixes: 4a476bd6d1d9 ("usbnet: New driver for QinHeng CH9200 devices") Cc: stable@vger.kernel.org Signed-off-by: Qasim Ijaz <qasdev00@gmail.com> --- drivers/net/mii.c | 2 ++ drivers/net/usb/ch9200.c | 7 +++++-- 2 files changed, 7 insertions(+), 2 deletions(-)