diff mbox series

[v2] net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read

Message ID 20231009-topic-dm9601_uninit_mdio_read-v2-1-f2fe39739b6c@gmail.com
State New
Headers
Series [v2] net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read |

Commit Message

Javier Carrasco Oct. 9, 2023, 10:26 p.m. UTC 
  syzbot has found an uninit-value bug triggered by the dm9601 driver [1].

This error happens because the variable res is not updated if the call
to dm_read_shared_word returns an error. In this particular case -EPROTO
was returned and res stayed uninitialized.

This can be avoided by checking the return value of dm_read_shared_word
and propagating the error if the read operation failed.

[1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
---
Changes in v2:
- Remove unnecessary 'err == 0' case
- Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
---
 drivers/net/usb/dm9601.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)


---
base-commit: 94f6f0550c625fab1f373bb86a6669b45e9748b3
change-id: 20231009-topic-dm9601_uninit_mdio_read-a15918ffbd6f

Best regards,

Comments

Greg KH Oct. 10, 2023, 6 a.m. UTC | #1 
On Tue, Oct 10, 2023 at 12:26:14AM +0200, Javier Carrasco wrote:
> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
> 
> This error happens because the variable res is not updated if the call
> to dm_read_shared_word returns an error. In this particular case -EPROTO
> was returned and res stayed uninitialized.
> 
> This can be avoided by checking the return value of dm_read_shared_word
> and propagating the error if the read operation failed.
> 
> [1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
> ---
> Changes in v2:
> - Remove unnecessary 'err == 0' case
> - Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
> ---
>  drivers/net/usb/dm9601.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)

What commit id does this fix?

thanks,

greg k-h
Peter Korsgaard Oct. 10, 2023, 6:19 a.m. UTC | #2 
>>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:

 > On Tue, Oct 10, 2023 at 12:26:14AM +0200, Javier Carrasco wrote:
 >> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
 >> 
 >> This error happens because the variable res is not updated if the call
 >> to dm_read_shared_word returns an error. In this particular case -EPROTO
 >> was returned and res stayed uninitialized.
 >> 
 >> This can be avoided by checking the return value of dm_read_shared_word
 >> and propagating the error if the read operation failed.
 >> 
 >> [1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
 >> 
 >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
 >> Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
 >> ---
 >> Changes in v2:
 >> - Remove unnecessary 'err == 0' case
 >> - Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
 >> ---
 >> drivers/net/usb/dm9601.c | 7 ++++++-
 >> 1 file changed, 6 insertions(+), 1 deletion(-)

 > What commit id does this fix?

It has been there since the beginning, so:

Fixes: d0374f4f9c35cdfbee0 ("USB: Davicom DM9601 usbnet driver")

Acked-by: Peter Korsgaard <peter@korsgaard.com>
Greg KH Oct. 10, 2023, 6:33 a.m. UTC | #3 
On Tue, Oct 10, 2023 at 08:19:48AM +0200, Peter Korsgaard wrote:
> >>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:
> 
>  > On Tue, Oct 10, 2023 at 12:26:14AM +0200, Javier Carrasco wrote:
>  >> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
>  >> 
>  >> This error happens because the variable res is not updated if the call
>  >> to dm_read_shared_word returns an error. In this particular case -EPROTO
>  >> was returned and res stayed uninitialized.
>  >> 
>  >> This can be avoided by checking the return value of dm_read_shared_word
>  >> and propagating the error if the read operation failed.
>  >> 
>  >> [1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
>  >> 
>  >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>  >> Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
>  >> ---
>  >> Changes in v2:
>  >> - Remove unnecessary 'err == 0' case
>  >> - Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
>  >> ---
>  >> drivers/net/usb/dm9601.c | 7 ++++++-
>  >> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
>  > What commit id does this fix?
> 
> It has been there since the beginning, so:
> 
> Fixes: d0374f4f9c35cdfbee0 ("USB: Davicom DM9601 usbnet driver")
> 
> Acked-by: Peter Korsgaard <peter@korsgaard.com>

Great, can someone add a cc: stable@ tag for this too please?

thanks,

greg k-h
Peter Korsgaard Oct. 10, 2023, 7:22 a.m. UTC | #4 
>>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:

 > On Tue, Oct 10, 2023 at 08:19:48AM +0200, Peter Korsgaard wrote:
 >> >>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:
 >> 
 >> > On Tue, Oct 10, 2023 at 12:26:14AM +0200, Javier Carrasco wrote:
 >> >> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
 >> >> 
 >> >> This error happens because the variable res is not updated if the call
 >> >> to dm_read_shared_word returns an error. In this particular case -EPROTO
 >> >> was returned and res stayed uninitialized.
 >> >> 
 >> >> This can be avoided by checking the return value of dm_read_shared_word
 >> >> and propagating the error if the read operation failed.
 >> >> 
 >> >> [1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
 >> >> 
 >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
 >> >> Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
 >> >> ---
 >> >> Changes in v2:
 >> >> - Remove unnecessary 'err == 0' case
 >> >> - Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
 >> >> ---
 >> >> drivers/net/usb/dm9601.c | 7 ++++++-
 >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
 >> 
 >> > What commit id does this fix?
 >> 
 >> It has been there since the beginning, so:
 >> 
 >> Fixes: d0374f4f9c35cdfbee0 ("USB: Davicom DM9601 usbnet driver")
 >> 
 >> Acked-by: Peter Korsgaard <peter@korsgaard.com>

 > Great, can someone add a cc: stable@ tag for this too please?

Cc: stable@vger.kernel.org
Greg KH Oct. 10, 2023, 7:28 a.m. UTC | #5 
On Tue, Oct 10, 2023 at 09:22:49AM +0200, Peter Korsgaard wrote:
> >>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:
> 
>  > On Tue, Oct 10, 2023 at 08:19:48AM +0200, Peter Korsgaard wrote:
>  >> >>>>> "Greg" == Greg KH <gregkh@linuxfoundation.org> writes:
>  >> 
>  >> > On Tue, Oct 10, 2023 at 12:26:14AM +0200, Javier Carrasco wrote:
>  >> >> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
>  >> >> 
>  >> >> This error happens because the variable res is not updated if the call
>  >> >> to dm_read_shared_word returns an error. In this particular case -EPROTO
>  >> >> was returned and res stayed uninitialized.
>  >> >> 
>  >> >> This can be avoided by checking the return value of dm_read_shared_word
>  >> >> and propagating the error if the read operation failed.
>  >> >> 
>  >> >> [1] https://syzkaller.appspot.com/bug?extid=1f53a30781af65d2c955
>  >> >> 
>  >> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>  >> >> Reported-and-tested-by: syzbot+1f53a30781af65d2c955@syzkaller.appspotmail.com
>  >> >> ---
>  >> >> Changes in v2:
>  >> >> - Remove unnecessary 'err == 0' case
>  >> >> - Link to v1: https://lore.kernel.org/r/20231009-topic-dm9601_uninit_mdio_read-v1-1-d4d775e24e3b@gmail.com
>  >> >> ---
>  >> >> drivers/net/usb/dm9601.c | 7 ++++++-
>  >> >> 1 file changed, 6 insertions(+), 1 deletion(-)
>  >> 
>  >> > What commit id does this fix?
>  >> 
>  >> It has been there since the beginning, so:
>  >> 
>  >> Fixes: d0374f4f9c35cdfbee0 ("USB: Davicom DM9601 usbnet driver")
>  >> 
>  >> Acked-by: Peter Korsgaard <peter@korsgaard.com>
> 
>  > Great, can someone add a cc: stable@ tag for this too please?
> 
> Cc: stable@vger.kernel.org

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>
patchwork-bot+netdevbpf@kernel.org Oct. 11, 2023, 3:10 a.m. UTC | #6 
Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 10 Oct 2023 00:26:14 +0200 you wrote:
> syzbot has found an uninit-value bug triggered by the dm9601 driver [1].
> 
> This error happens because the variable res is not updated if the call
> to dm_read_shared_word returns an error. In this particular case -EPROTO
> was returned and res stayed uninitialized.
> 
> This can be avoided by checking the return value of dm_read_shared_word
> and propagating the error if the read operation failed.
> 
> [...]

Here is the summary with links:
  - [v2] net: usb: dm9601: fix uninitialized variable use in dm9601_mdio_read
    https://git.kernel.org/netdev/net/c/8f8abb863fa5

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/usb/dm9601.c b/drivers/net/usb/dm9601.c
index 48d7d278631e..99ec1d4a972d 100644
--- a/drivers/net/usb/dm9601.c
+++ b/drivers/net/usb/dm9601.c
@@ -222,13 +222,18 @@  static int dm9601_mdio_read(struct net_device *netdev, int phy_id, int loc)
 	struct usbnet *dev = netdev_priv(netdev);
 
 	__le16 res;
+	int err;
 
 	if (phy_id) {
 		netdev_dbg(dev->net, "Only internal phy supported\n");
 		return 0;
 	}
 
-	dm_read_shared_word(dev, 1, loc, &res);
+	err = dm_read_shared_word(dev, 1, loc, &res);
+	if (err < 0) {
+		netdev_err(dev->net, "MDIO read error: %d\n", err);
+		return err;
+	}
 
 	netdev_dbg(dev->net,
 		   "dm9601_mdio_read() phy_id=0x%02x, loc=0x%02x, returns=0x%04x\n",