[PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Pavel Skripkin]

Syzbot reported uninit value pegasus_probe(). The problem was in missing
error handling.

get_interrupt_interval() internally calls read_eprom_word() which can
fail in some cases. For example: failed to receive usb control message.
These cases should be handled to prevent uninit value bug, since
read_eprom_word() will not initialize passed stack variable in case of
internal failure.

Fail log:

BUG: KMSAN: uninit-value in get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
BUG: KMSAN: uninit-value in pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
CPU: 1 PID: 825 Comm: kworker/1:1 Not tainted 5.12.0-rc6-syzkaller #0
...
Workqueue: usb_hub_wq hub_event
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x24c/0x2e0 lib/dump_stack.c:120
 kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
 __msan_warning+0x5c/0xa0 mm/kmsan/kmsan_instr.c:197
 get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
 pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
....

Local variable ----data.i@pegasus_probe created at:
 get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
 pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
 get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
 pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152

Reported-and-tested-by: syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---
 drivers/net/usb/pegasus.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 9a907182569c..bc2dbf86496b 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -735,12 +735,16 @@ static inline void disable_net_traffic(pegasus_t *pegasus)
 	set_registers(pegasus, EthCtrl0, sizeof(tmp), &tmp);
 }
 
-static inline void get_interrupt_interval(pegasus_t *pegasus)
+static inline int get_interrupt_interval(pegasus_t *pegasus)
 {
 	u16 data;
 	u8 interval;
+	int ret;
+
+	ret = read_eprom_word(pegasus, 4, &data);
+	if (ret < 0)
+		return ret;
 
-	read_eprom_word(pegasus, 4, &data);
 	interval = data >> 8;
 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
 		if (interval < 0x80) {
@@ -755,6 +759,8 @@ static inline void get_interrupt_interval(pegasus_t *pegasus)
 		}
 	}
 	pegasus->intr_interval = interval;
+
+	return 0;
 }
 
 static void set_carrier(struct net_device *net)
@@ -1149,7 +1155,9 @@ static int pegasus_probe(struct usb_interface *intf,
 				| NETIF_MSG_PROBE | NETIF_MSG_LINK);
 
 	pegasus->features = usb_dev_id[dev_index].private;
-	get_interrupt_interval(pegasus);
+	res = get_interrupt_interval(pegasus);
+	if (res)
+		goto out2;
 	if (reset_mac(pegasus)) {
 		dev_err(&intf->dev, "can't reset MAC\n");
 		res = -EIO;
-- 
2.32.0

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Petko Manolov]

On 21-07-31 00:44:11, Pavel Skripkin wrote:
> Syzbot reported uninit value pegasus_probe(). The problem was in missing
> error handling.
> 
> get_interrupt_interval() internally calls read_eprom_word() which can
> fail in some cases. For example: failed to receive usb control message.
> These cases should be handled to prevent uninit value bug, since
> read_eprom_word() will not initialize passed stack variable in case of
> internal failure.

Well, this is most definitelly a bug.

ACK!


		Petko


> Fail log:
> 
> BUG: KMSAN: uninit-value in get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
> BUG: KMSAN: uninit-value in pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> CPU: 1 PID: 825 Comm: kworker/1:1 Not tainted 5.12.0-rc6-syzkaller #0
> ...
> Workqueue: usb_hub_wq hub_event
> Call Trace:
>  __dump_stack lib/dump_stack.c:79 [inline]
>  dump_stack+0x24c/0x2e0 lib/dump_stack.c:120
>  kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
>  __msan_warning+0x5c/0xa0 mm/kmsan/kmsan_instr.c:197
>  get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
>  pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> ....
> 
> Local variable ----data.i@pegasus_probe created at:
>  get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
>  pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
>  get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
>  pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
> 
> Reported-and-tested-by: syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>  drivers/net/usb/pegasus.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9a907182569c..bc2dbf86496b 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -735,12 +735,16 @@ static inline void disable_net_traffic(pegasus_t *pegasus)
>  	set_registers(pegasus, EthCtrl0, sizeof(tmp), &tmp);
>  }
>  
> -static inline void get_interrupt_interval(pegasus_t *pegasus)
> +static inline int get_interrupt_interval(pegasus_t *pegasus)
>  {
>  	u16 data;
>  	u8 interval;
> +	int ret;
> +
> +	ret = read_eprom_word(pegasus, 4, &data);
> +	if (ret < 0)
> +		return ret;
>  
> -	read_eprom_word(pegasus, 4, &data);
>  	interval = data >> 8;
>  	if (pegasus->usb->speed != USB_SPEED_HIGH) {
>  		if (interval < 0x80) {
> @@ -755,6 +759,8 @@ static inline void get_interrupt_interval(pegasus_t *pegasus)
>  		}
>  	}
>  	pegasus->intr_interval = interval;
> +
> +	return 0;
>  }
>  
>  static void set_carrier(struct net_device *net)
> @@ -1149,7 +1155,9 @@ static int pegasus_probe(struct usb_interface *intf,
>  				| NETIF_MSG_PROBE | NETIF_MSG_LINK);
>  
>  	pegasus->features = usb_dev_id[dev_index].private;
> -	get_interrupt_interval(pegasus);
> +	res = get_interrupt_interval(pegasus);
> +	if (res)
> +		goto out2;
>  	if (reset_mac(pegasus)) {
>  		dev_err(&intf->dev, "can't reset MAC\n");
>  		res = -EIO;
> -- 
> 2.32.0
> 
> 

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Pavel Skripkin]

On Sun, 1 Aug 2021 15:36:27 +0300
Petko Manolov <petkan@nucleusys.com> wrote:

> On 21-07-31 00:44:11, Pavel Skripkin wrote:
> > Syzbot reported uninit value pegasus_probe(). The problem was in
> > missing error handling.
> > 
> > get_interrupt_interval() internally calls read_eprom_word() which
> > can fail in some cases. For example: failed to receive usb control
> > message. These cases should be handled to prevent uninit value bug,
> > since read_eprom_word() will not initialize passed stack variable
> > in case of internal failure.
> 
> Well, this is most definitelly a bug.
> 
> ACK!
> 
> 
> 		Petko
> 
> 

Thank you, Petko!


BTW: I found a lot uses of {get,set}_registers without error
checking. I think, some of them could be fixed easily (like in
enable_eprom_write), but, I guess, disable_eprom_write is not so easy.
For example, if we cannot disable eprom should we retry? If not, will
device get in some unexpected state?

Im not familiar with this device, but I can prepare a patch to wrap all
these calls with proper error checking



With regards,
Pavel Skripkin

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Petko Manolov]

On 21-08-01 22:35:13, Pavel Skripkin wrote:
> On Sun, 1 Aug 2021 15:36:27 +0300 Petko Manolov <petkan@nucleusys.com> wrote:
> 
> > On 21-07-31 00:44:11, Pavel Skripkin wrote:
> > > Syzbot reported uninit value pegasus_probe(). The problem was in missing
> > > error handling.
> > > 
> > > get_interrupt_interval() internally calls read_eprom_word() which can fail
> > > in some cases. For example: failed to receive usb control message. These
> > > cases should be handled to prevent uninit value bug, since
> > > read_eprom_word() will not initialize passed stack variable in case of
> > > internal failure.
> > 
> > Well, this is most definitelly a bug.
> > 
> > ACK!
> > 
> > 
> >		Petko
> 
> BTW: I found a lot uses of {get,set}_registers without error checking. I
> think, some of them could be fixed easily (like in enable_eprom_write), but, I
> guess, disable_eprom_write is not so easy. For example, if we cannot disable
> eprom should we retry? If not, will device get in some unexpected state?

Everything bracketed by PEGASUS_WRITE_EEPROM is more or less dead code.  I've
added this feature because the chip give us the ability to write to the flash,
but i seriously doubt anybody ever used it.  Come to think about it, i should
just remove this code.

> Im not familiar with this device, but I can prepare a patch to wrap all these
> calls with proper error checking

Well, i've stared at the code a bit and i see some places where not checking the
error returned by {get,set}_registers() could really be problematic.  I'll cook
a patch with what i think needs doing and will submit it here for review.


cheers,
Petko

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Petko Manolov]

On 21-08-01 22:35:13, Pavel Skripkin wrote:
> On Sun, 1 Aug 2021 15:36:27 +0300
> Petko Manolov <petkan@nucleusys.com> wrote:
> 
> > On 21-07-31 00:44:11, Pavel Skripkin wrote:
> > > Syzbot reported uninit value pegasus_probe(). The problem was in missing
> > > error handling.
> > > 
> > > get_interrupt_interval() internally calls read_eprom_word() which can fail
> > > in some cases. For example: failed to receive usb control message. These
> > > cases should be handled to prevent uninit value bug, since
> > > read_eprom_word() will not initialize passed stack variable in case of
> > > internal failure.
> > 
> > Well, this is most definitelly a bug.
> > 
> > ACK!
> > 
> > 
> > 		Petko
> > 
> > 
> 
> Thank you, Petko!
> 
> 
> BTW: I found a lot uses of {get,set}_registers without error checking. I
> think, some of them could be fixed easily (like in enable_eprom_write), but, I
> guess, disable_eprom_write is not so easy. For example, if we cannot disable
> eprom should we retry? If not, will device get in some unexpected state?
> 
> Im not familiar with this device, but I can prepare a patch to wrap all these
> calls with proper error checking

Here goes a preliminary patch that should apply on top of your, maybe with just
a few warnings.  This is a review only diff, not the real patch.  It's against
5.14-rc4.

I am mildly curious why syzbot didn't catch the same type of bug in
enable_net_traffic() and setup_pegasus_II() for example.


		Petko

---

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index 9a907182569c..eafbe8107907 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -26,6 +26,8 @@
  *		...
  *		v0.9.3	simplified [get|set]_register(s), async update registers
  *			logic revisited, receive skb_pool removed.
+ *		v1.0.1	add error checking for set_register(s)(), see if calling
+ *			get_registers() has failed and print a message accordingly.
  */
 
 #include <linux/sched.h>
@@ -45,7 +47,7 @@
 /*
  * Version Information
  */
-#define DRIVER_VERSION "v0.9.3 (2013/04/25)"
+#define DRIVER_VERSION "v1.0.1 (2021/08/01)"
 #define DRIVER_AUTHOR "Petko Manolov <petkan@nucleusys.com>"
 #define DRIVER_DESC "Pegasus/Pegasus II USB Ethernet driver"
 
@@ -132,9 +134,15 @@ static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
 static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
 			 const void *data)
 {
-	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
+	int ret;
+
+	ret = usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
 				    PEGASUS_REQT_WRITE, 0, indx, data, size,
 				    1000, GFP_NOIO);
+	if (ret < 0)
+		netif_dbg(pegasus, drv, pegasus->net, "%s failed with %d\n", __func__, ret);
+
+	return ret;
 }
 
 /*
@@ -145,10 +153,15 @@ static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
 static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
 {
 	void *buf = &data;
+	int ret;
 
-	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REG,
+	ret = usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REG,
 				    PEGASUS_REQT_WRITE, data, indx, buf, 1,
 				    1000, GFP_NOIO);
+	if (ret < 0)
+		netif_dbg(pegasus, drv, pegasus->net, "%s failed with %d\n", __func__, ret);
+
+	return ret;
 }
 
 static int update_eth_regs_async(pegasus_t *pegasus)
@@ -188,10 +201,9 @@ static int update_eth_regs_async(pegasus_t *pegasus)
 
 static int __mii_op(pegasus_t *p, __u8 phy, __u8 indx, __u16 *regd, __u8 cmd)
 {
-	int i;
-	__u8 data[4] = { phy, 0, 0, indx };
+	int i, ret = -ETIMEDOUT;
 	__le16 regdi;
-	int ret = -ETIMEDOUT;
+	__u8 data[4] = { phy, 0, 0, indx };
 
 	if (cmd & PHY_WRITE) {
 		__le16 *t = (__le16 *) & data[1];
@@ -211,8 +223,9 @@ static int __mii_op(pegasus_t *p, __u8 phy, __u8 indx, __u16 *regd, __u8 cmd)
 		goto fail;
 	if (cmd & PHY_READ) {
 		ret = get_registers(p, PhyData, 2, &regdi);
+		if (ret < 0)
+			goto fail;
 		*regd = le16_to_cpu(regdi);
-		return ret;
 	}
 	return 0;
 fail:
@@ -235,9 +248,13 @@ static int write_mii_word(pegasus_t *pegasus, __u8 phy, __u8 indx, __u16 *regd)
 static int mdio_read(struct net_device *dev, int phy_id, int loc)
 {
 	pegasus_t *pegasus = netdev_priv(dev);
+	int ret;
 	u16 res;
 
-	read_mii_word(pegasus, phy_id, loc, &res);
+	ret = read_mii_word(pegasus, phy_id, loc, &res);
+	if (ret < 0)
+		return ret;
+
 	return (int)res;
 }
 
@@ -251,10 +268,9 @@ static void mdio_write(struct net_device *dev, int phy_id, int loc, int val)
 
 static int read_eprom_word(pegasus_t *pegasus, __u8 index, __u16 *retdata)
 {
-	int i;
-	__u8 tmp = 0;
+	int ret, i;
 	__le16 retdatai;
-	int ret;
+	__u8 tmp = 0;
 
 	set_register(pegasus, EpromCtrl, 0);
 	set_register(pegasus, EpromOffset, index);
@@ -262,21 +278,25 @@ static int read_eprom_word(pegasus_t *pegasus, __u8 index, __u16 *retdata)
 
 	for (i = 0; i < REG_TIMEOUT; i++) {
 		ret = get_registers(pegasus, EpromCtrl, 1, &tmp);
+		if (ret < 0)
+			goto fail;
 		if (tmp & EPROM_DONE)
 			break;
-		if (ret == -ESHUTDOWN)
-			goto fail;
 	}
-	if (i >= REG_TIMEOUT)
+	if (i >= REG_TIMEOUT) {
+		ret = -ETIMEDOUT;
 		goto fail;
+	}
 
 	ret = get_registers(pegasus, EpromData, 2, &retdatai);
+	if (ret < 0)
+		goto fail;
 	*retdata = le16_to_cpu(retdatai);
 	return ret;
 
 fail:
-	netif_warn(pegasus, drv, pegasus->net, "%s failed\n", __func__);
-	return -ETIMEDOUT;
+	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
+	return ret;
 }
 
 #ifdef	PEGASUS_WRITE_EEPROM
@@ -324,10 +344,10 @@ static int write_eprom_word(pegasus_t *pegasus, __u8 index, __u16 data)
 	return ret;
 
 fail:
-	netif_warn(pegasus, drv, pegasus->net, "%s failed\n", __func__);
+	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
 	return -ETIMEDOUT;
 }
-#endif				/* PEGASUS_WRITE_EEPROM */
+#endif	/* PEGASUS_WRITE_EEPROM */
 
 static inline int get_node_id(pegasus_t *pegasus, u8 *id)
 {
@@ -367,19 +387,21 @@ static void set_ethernet_addr(pegasus_t *pegasus)
 	return;
 err:
 	eth_hw_addr_random(pegasus->net);
-	dev_info(&pegasus->intf->dev, "software assigned MAC address.\n");
+	netif_dbg(pegasus, drv, pegasus->net, "software assigned MAC address.\n");
 
 	return;
 }
 
 static inline int reset_mac(pegasus_t *pegasus)
 {
+	int ret, i;
 	__u8 data = 0x8;
-	int i;
 
 	set_register(pegasus, EthCtrl1, data);
 	for (i = 0; i < REG_TIMEOUT; i++) {
-		get_registers(pegasus, EthCtrl1, 1, &data);
+		ret = get_registers(pegasus, EthCtrl1, 1, &data);
+		if (ret < 0)
+			goto fail;
 		if (~data & 0x08) {
 			if (loopback)
 				break;
@@ -402,22 +424,29 @@ static inline int reset_mac(pegasus_t *pegasus)
 	}
 	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_ELCON) {
 		__u16 auxmode;
-		read_mii_word(pegasus, 3, 0x1b, &auxmode);
+		ret = read_mii_word(pegasus, 3, 0x1b, &auxmode);
+		if (ret < 0)
+			goto fail;
 		auxmode |= 4;
 		write_mii_word(pegasus, 3, 0x1b, &auxmode);
 	}
 
 	return 0;
+fail:
+	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
+	return ret;
 }
 
 static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 {
-	__u16 linkpart;
-	__u8 data[4];
 	pegasus_t *pegasus = netdev_priv(dev);
 	int ret;
+	__u16 linkpart;
+	__u8 data[4];
 
-	read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
+	ret = read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
+	if (ret < 0)
+		goto fail;
 	data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
 	data[1] = 0;
 	if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
@@ -435,11 +464,16 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
 	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
 	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
 		u16 auxmode;
-		read_mii_word(pegasus, 0, 0x1b, &auxmode);
+		ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
+		if (ret < 0)
+			goto fail;
 		auxmode |= 4;
 		write_mii_word(pegasus, 0, 0x1b, &auxmode);
 	}
 
+	return 0;
+fail:
+	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
 	return ret;
 }
 
@@ -447,9 +481,9 @@ static void read_bulk_callback(struct urb *urb)
 {
 	pegasus_t *pegasus = urb->context;
 	struct net_device *net;
+	u8 *buf = urb->transfer_buffer;
 	int rx_status, count = urb->actual_length;
 	int status = urb->status;
-	u8 *buf = urb->transfer_buffer;
 	__u16 pkt_len;
 
 	if (!pegasus)
@@ -1049,6 +1083,7 @@ static __u8 mii_phy_probe(pegasus_t *pegasus)
 
 static inline void setup_pegasus_II(pegasus_t *pegasus)
 {
+	int ret;
 	__u8 data = 0xa5;
 
 	set_register(pegasus, Reg1d, 0);
@@ -1060,7 +1095,9 @@ static inline void setup_pegasus_II(pegasus_t *pegasus)
 		set_register(pegasus, Reg7b, 2);
 
 	set_register(pegasus, 0x83, data);
-	get_registers(pegasus, 0x83, 1, &data);
+	ret = get_registers(pegasus, 0x83, 1, &data);
+	if (ret < 0)
+		goto fail;
 
 	if (data == 0xa5)
 		pegasus->chip = 0x8513;
@@ -1075,6 +1112,8 @@ static inline void setup_pegasus_II(pegasus_t *pegasus)
 		set_register(pegasus, Reg81, 6);
 	else
 		set_register(pegasus, Reg81, 2);
+fail:
+	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
 }
 
 static void check_carrier(struct work_struct *work)

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Pavel Skripkin]

On 8/2/21 11:07 PM, Petko Manolov wrote:
> On 21-08-01 22:35:13, Pavel Skripkin wrote:
>> On Sun, 1 Aug 2021 15:36:27 +0300
>> Petko Manolov <petkan@nucleusys.com> wrote:
>> 
>> > On 21-07-31 00:44:11, Pavel Skripkin wrote:
>> > > Syzbot reported uninit value pegasus_probe(). The problem was in missing
>> > > error handling.
>> > > 
>> > > get_interrupt_interval() internally calls read_eprom_word() which can fail
>> > > in some cases. For example: failed to receive usb control message. These
>> > > cases should be handled to prevent uninit value bug, since
>> > > read_eprom_word() will not initialize passed stack variable in case of
>> > > internal failure.
>> > 
>> > Well, this is most definitelly a bug.
>> > 
>> > ACK!
>> > 
>> > 
>> > 		Petko
>> > 
>> > 
>> 
>> Thank you, Petko!
>> 
>> 
>> BTW: I found a lot uses of {get,set}_registers without error checking. I
>> think, some of them could be fixed easily (like in enable_eprom_write), but, I
>> guess, disable_eprom_write is not so easy. For example, if we cannot disable
>> eprom should we retry? If not, will device get in some unexpected state?
>> 
>> Im not familiar with this device, but I can prepare a patch to wrap all these
>> calls with proper error checking
> 
> Here goes a preliminary patch that should apply on top of your, maybe with just
> a few warnings.  This is a review only diff, not the real patch.  It's against
> 5.14-rc4.
> 
> I am mildly curious why syzbot didn't catch the same type of bug in
> enable_net_traffic() and setup_pegasus_II() for example.
> 
> 
> 		Petko
> 
> ---
> 
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9a907182569c..eafbe8107907 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -26,6 +26,8 @@
>    *		...
>    *		v0.9.3	simplified [get|set]_register(s), async update registers
>    *			logic revisited, receive skb_pool removed.
> + *		v1.0.1	add error checking for set_register(s)(), see if calling
> + *			get_registers() has failed and print a message accordingly.
>    */
>   
>   #include <linux/sched.h>
> @@ -45,7 +47,7 @@
>   /*
>    * Version Information
>    */
> -#define DRIVER_VERSION "v0.9.3 (2013/04/25)"
> +#define DRIVER_VERSION "v1.0.1 (2021/08/01)"
>   #define DRIVER_AUTHOR "Petko Manolov <petkan@nucleusys.com>"
>   #define DRIVER_DESC "Pegasus/Pegasus II USB Ethernet driver"
>   
> @@ -132,9 +134,15 @@ static int get_registers(pegasus_t *pegasus, __u16 indx, __u16 size, void *data)
>   static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
>   			 const void *data)
>   {
> -	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
> +	int ret;
> +
> +	ret = usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REGS,
>   				    PEGASUS_REQT_WRITE, 0, indx, data, size,
>   				    1000, GFP_NOIO);
> +	if (ret < 0)
> +		netif_dbg(pegasus, drv, pegasus->net, "%s failed with %d\n", __func__, ret);
> +
> +	return ret;
>   }
>   
>   /*
> @@ -145,10 +153,15 @@ static int set_registers(pegasus_t *pegasus, __u16 indx, __u16 size,
>   static int set_register(pegasus_t *pegasus, __u16 indx, __u8 data)
>   {
>   	void *buf = &data;
> +	int ret;
>   
> -	return usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REG,
> +	ret = usb_control_msg_send(pegasus->usb, 0, PEGASUS_REQ_SET_REG,
>   				    PEGASUS_REQT_WRITE, data, indx, buf, 1,
>   				    1000, GFP_NOIO);
> +	if (ret < 0)
> +		netif_dbg(pegasus, drv, pegasus->net, "%s failed with %d\n", __func__, ret);
> +
> +	return ret;
>   }
>   
>   static int update_eth_regs_async(pegasus_t *pegasus)
> @@ -188,10 +201,9 @@ static int update_eth_regs_async(pegasus_t *pegasus)
>   
>   static int __mii_op(pegasus_t *p, __u8 phy, __u8 indx, __u16 *regd, __u8 cmd)
>   {
> -	int i;
> -	__u8 data[4] = { phy, 0, 0, indx };
> +	int i, ret = -ETIMEDOUT;
>   	__le16 regdi;
> -	int ret = -ETIMEDOUT;
> +	__u8 data[4] = { phy, 0, 0, indx };
>   
>   	if (cmd & PHY_WRITE) {
>   		__le16 *t = (__le16 *) & data[1];
> @@ -211,8 +223,9 @@ static int __mii_op(pegasus_t *p, __u8 phy, __u8 indx, __u16 *regd, __u8 cmd)
>   		goto fail;
>   	if (cmd & PHY_READ) {
>   		ret = get_registers(p, PhyData, 2, &regdi);
> +		if (ret < 0)
> +			goto fail;
>   		*regd = le16_to_cpu(regdi);
> -		return ret;
>   	}
>   	return 0;
>   fail:
> @@ -235,9 +248,13 @@ static int write_mii_word(pegasus_t *pegasus, __u8 phy, __u8 indx, __u16 *regd)
>   static int mdio_read(struct net_device *dev, int phy_id, int loc)
>   {
>   	pegasus_t *pegasus = netdev_priv(dev);
> +	int ret;
>   	u16 res;
>   
> -	read_mii_word(pegasus, phy_id, loc, &res);
> +	ret = read_mii_word(pegasus, phy_id, loc, &res);
> +	if (ret < 0)
> +		return ret;
> +
>   	return (int)res;
>   }
>   
> @@ -251,10 +268,9 @@ static void mdio_write(struct net_device *dev, int phy_id, int loc, int val)
>   
>   static int read_eprom_word(pegasus_t *pegasus, __u8 index, __u16 *retdata)
>   {
> -	int i;
> -	__u8 tmp = 0;
> +	int ret, i;
>   	__le16 retdatai;
> -	int ret;
> +	__u8 tmp = 0;
>   
>   	set_register(pegasus, EpromCtrl, 0);
>   	set_register(pegasus, EpromOffset, index);
> @@ -262,21 +278,25 @@ static int read_eprom_word(pegasus_t *pegasus, __u8 index, __u16 *retdata)
>   
>   	for (i = 0; i < REG_TIMEOUT; i++) {
>   		ret = get_registers(pegasus, EpromCtrl, 1, &tmp);
> +		if (ret < 0)
> +			goto fail;
>   		if (tmp & EPROM_DONE)
>   			break;
> -		if (ret == -ESHUTDOWN)
> -			goto fail;
>   	}
> -	if (i >= REG_TIMEOUT)
> +	if (i >= REG_TIMEOUT) {
> +		ret = -ETIMEDOUT;
>   		goto fail;
> +	}
>   
>   	ret = get_registers(pegasus, EpromData, 2, &retdatai);
> +	if (ret < 0)
> +		goto fail;
>   	*retdata = le16_to_cpu(retdatai);
>   	return ret;
>   
>   fail:
> -	netif_warn(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> -	return -ETIMEDOUT;
> +	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> +	return ret;
>   }
>   
>   #ifdef	PEGASUS_WRITE_EEPROM
> @@ -324,10 +344,10 @@ static int write_eprom_word(pegasus_t *pegasus, __u8 index, __u16 data)
>   	return ret;
>   
>   fail:
> -	netif_warn(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> +	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
>   	return -ETIMEDOUT;
>   }
> -#endif				/* PEGASUS_WRITE_EEPROM */
> +#endif	/* PEGASUS_WRITE_EEPROM */
>   
>   static inline int get_node_id(pegasus_t *pegasus, u8 *id)
>   {
> @@ -367,19 +387,21 @@ static void set_ethernet_addr(pegasus_t *pegasus)
>   	return;
>   err:
>   	eth_hw_addr_random(pegasus->net);
> -	dev_info(&pegasus->intf->dev, "software assigned MAC address.\n");
> +	netif_dbg(pegasus, drv, pegasus->net, "software assigned MAC address.\n");
>   
>   	return;
>   }

Not related to the patch, but, maybe, we should remove this return?

>   
>   static inline int reset_mac(pegasus_t *pegasus)
>   {
> +	int ret, i;
>   	__u8 data = 0x8;
> -	int i;
>   
>   	set_register(pegasus, EthCtrl1, data);
>   	for (i = 0; i < REG_TIMEOUT; i++) {
> -		get_registers(pegasus, EthCtrl1, 1, &data);
> +		ret = get_registers(pegasus, EthCtrl1, 1, &data);
> +		if (ret < 0)
> +			goto fail;
>   		if (~data & 0x08) {
>   			if (loopback)
>   				break;
> @@ -402,22 +424,29 @@ static inline int reset_mac(pegasus_t *pegasus)
>   	}
>   	if (usb_dev_id[pegasus->dev_index].vendor == VENDOR_ELCON) {
>   		__u16 auxmode;
> -		read_mii_word(pegasus, 3, 0x1b, &auxmode);
> +		ret = read_mii_word(pegasus, 3, 0x1b, &auxmode);
> +		if (ret < 0)
> +			goto fail;
>   		auxmode |= 4;
>   		write_mii_word(pegasus, 3, 0x1b, &auxmode);
>   	}
>   
>   	return 0;
> +fail:
> +	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
> +	return ret;
>   }
>   
>   static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
>   {
> -	__u16 linkpart;
> -	__u8 data[4];
>   	pegasus_t *pegasus = netdev_priv(dev);
>   	int ret;
> +	__u16 linkpart;
> +	__u8 data[4];
>   
> -	read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
> +	ret = read_mii_word(pegasus, pegasus->phy, MII_LPA, &linkpart);
> +	if (ret < 0)
> +		goto fail;
>   	data[0] = 0xc8; /* TX & RX enable, append status, no CRC */
>   	data[1] = 0;
>   	if (linkpart & (ADVERTISE_100FULL | ADVERTISE_10FULL))
> @@ -435,11 +464,16 @@ static int enable_net_traffic(struct net_device *dev, struct usb_device *usb)
>   	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_LINKSYS2 ||
>   	    usb_dev_id[pegasus->dev_index].vendor == VENDOR_DLINK) {
>   		u16 auxmode;
> -		read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +		ret = read_mii_word(pegasus, 0, 0x1b, &auxmode);
> +		if (ret < 0)
> +			goto fail;
>   		auxmode |= 4;
>   		write_mii_word(pegasus, 0, 0x1b, &auxmode);
>   	}
>   
> +	return 0;
> +fail:
> +	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
>   	return ret;
>   }
>   
> @@ -447,9 +481,9 @@ static void read_bulk_callback(struct urb *urb)
>   {
>   	pegasus_t *pegasus = urb->context;
>   	struct net_device *net;
> +	u8 *buf = urb->transfer_buffer;
>   	int rx_status, count = urb->actual_length;
>   	int status = urb->status;
> -	u8 *buf = urb->transfer_buffer;
>   	__u16 pkt_len;
>   
>   	if (!pegasus)
> @@ -1049,6 +1083,7 @@ static __u8 mii_phy_probe(pegasus_t *pegasus)
>   
>   static inline void setup_pegasus_II(pegasus_t *pegasus)
>   {
> +	int ret;
>   	__u8 data = 0xa5;
>   
>   	set_register(pegasus, Reg1d, 0);
> @@ -1060,7 +1095,9 @@ static inline void setup_pegasus_II(pegasus_t *pegasus)
>   		set_register(pegasus, Reg7b, 2);
>   
>   	set_register(pegasus, 0x83, data);
> -	get_registers(pegasus, 0x83, 1, &data);
> +	ret = get_registers(pegasus, 0x83, 1, &data);
> +	if (ret < 0)
> +		goto fail;
>   
>   	if (data == 0xa5)
>   		pegasus->chip = 0x8513;
> @@ -1075,6 +1112,8 @@ static inline void setup_pegasus_II(pegasus_t *pegasus)
>   		set_register(pegasus, Reg81, 6);
>   	else
>   		set_register(pegasus, Reg81, 2);
> +fail:
> +	netif_dbg(pegasus, drv, pegasus->net, "%s failed\n", __func__);
>   }
>   
>   static void check_carrier(struct work_struct *work)
> 

Looks good to me.

Build test didn't generate any warnings (tested on top of v5.14-rc4 with 
yours and mine patches applied). Smatch didn't generate any warnings as 
well.

I found two more places, where read_mii_word() is used without error 
checking: pegasus_ioctl() and mii_phy_probe(). If I understand 
correctly, mii_phy_probe() is more dangerous one, since it's used in 
->probe().


With regards,
Pavel Skripkin

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Greg KH]

On Mon, Aug 02, 2021 at 11:07:23PM +0300, Petko Manolov wrote:
> diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
> index 9a907182569c..eafbe8107907 100644
> --- a/drivers/net/usb/pegasus.c
> +++ b/drivers/net/usb/pegasus.c
> @@ -26,6 +26,8 @@
>   *		...
>   *		v0.9.3	simplified [get|set]_register(s), async update registers
>   *			logic revisited, receive skb_pool removed.
> + *		v1.0.1	add error checking for set_register(s)(), see if calling
> + *			get_registers() has failed and print a message accordingly.
>   */
>  
>  #include <linux/sched.h>
> @@ -45,7 +47,7 @@
>  /*
>   * Version Information
>   */
> -#define DRIVER_VERSION "v0.9.3 (2013/04/25)"
> +#define DRIVER_VERSION "v1.0.1 (2021/08/01)"

Nit, the log above, and the driver version here, mean nothing when it
comes to code in the kernel tree, both should be dropped as we have full
kernel changelog through git, and the version is bound to the kernel
release the driver came in.

thanks,

greg k-h

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Pavel Skripkin]

On 7/31/21 12:44 AM, Pavel Skripkin wrote:
> Syzbot reported uninit value pegasus_probe(). The problem was in missing
> error handling.
> 
> get_interrupt_interval() internally calls read_eprom_word() which can
> fail in some cases. For example: failed to receive usb control message.
> These cases should be handled to prevent uninit value bug, since
> read_eprom_word() will not initialize passed stack variable in case of
> internal failure.
> 
> Fail log:
> 
> BUG: KMSAN: uninit-value in get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
> BUG: KMSAN: uninit-value in pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> CPU: 1 PID: 825 Comm: kworker/1:1 Not tainted 5.12.0-rc6-syzkaller #0
> ...
> Workqueue: usb_hub_wq hub_event
> Call Trace:
>   __dump_stack lib/dump_stack.c:79 [inline]
>   dump_stack+0x24c/0x2e0 lib/dump_stack.c:120
>   kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
>   __msan_warning+0x5c/0xa0 mm/kmsan/kmsan_instr.c:197
>   get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
>   pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> ....
> 
> Local variable ----data.i@pegasus_probe created at:
>   get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
>   pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
>   get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
>   pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
> 
> Reported-and-tested-by: syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> ---
>   drivers/net/usb/pegasus.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)
> 

Hi, David and Jakub!

Should I rebase this patch on top of Petko's clean-up patches? :

1. https://git.kernel.org/netdev/net/c/8a160e2e9aeb
2. https://git.kernel.org/netdev/net/c/bc65bacf239d



With regards,
Pavel Skripkin

Re: [PATCH] net: pegasus: fix uninit-value in get_interrupt_interval

[Jakub Kicinski]

On Wed, 4 Aug 2021 13:44:05 +0300 Pavel Skripkin wrote:
> On 7/31/21 12:44 AM, Pavel Skripkin wrote:
> > Syzbot reported uninit value pegasus_probe(). The problem was in missing
> > error handling.
> > 
> > get_interrupt_interval() internally calls read_eprom_word() which can
> > fail in some cases. For example: failed to receive usb control message.
> > These cases should be handled to prevent uninit value bug, since
> > read_eprom_word() will not initialize passed stack variable in case of
> > internal failure.
> > 
> > Fail log:
> > 
> > BUG: KMSAN: uninit-value in get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
> > BUG: KMSAN: uninit-value in pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> > CPU: 1 PID: 825 Comm: kworker/1:1 Not tainted 5.12.0-rc6-syzkaller #0
> > ...
> > Workqueue: usb_hub_wq hub_event
> > Call Trace:
> >   __dump_stack lib/dump_stack.c:79 [inline]
> >   dump_stack+0x24c/0x2e0 lib/dump_stack.c:120
> >   kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
> >   __msan_warning+0x5c/0xa0 mm/kmsan/kmsan_instr.c:197
> >   get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
> >   pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
> > ....
> > 
> > Local variable ----data.i@pegasus_probe created at:
> >   get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
> >   pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
> >   get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
> >   pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
> > 
> > Reported-and-tested-by: syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
> 
> Hi, David and Jakub!
> 
> Should I rebase this patch on top of Petko's clean-up patches? :
> 
> 1. https://git.kernel.org/netdev/net/c/8a160e2e9aeb
> 2. https://git.kernel.org/netdev/net/c/bc65bacf239d

Yes, rebase on top of net, the patches are there. Please mark the new
submission as [PATCH net v2].

[PATCH net v2] net: pegasus: fix uninit-value in get_interrupt_interval

[Pavel Skripkin]

Syzbot reported uninit value pegasus_probe(). The problem was in missing
error handling.

get_interrupt_interval() internally calls read_eprom_word() which can
fail in some cases. For example: failed to receive usb control message.
These cases should be handled to prevent uninit value bug, since
read_eprom_word() will not initialize passed stack variable in case of
internal failure.

Fail log:

BUG: KMSAN: uninit-value in get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
BUG: KMSAN: uninit-value in pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
CPU: 1 PID: 825 Comm: kworker/1:1 Not tainted 5.12.0-rc6-syzkaller #0
...
Workqueue: usb_hub_wq hub_event
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x24c/0x2e0 lib/dump_stack.c:120
 kmsan_report+0xfb/0x1e0 mm/kmsan/kmsan_report.c:118
 __msan_warning+0x5c/0xa0 mm/kmsan/kmsan_instr.c:197
 get_interrupt_interval drivers/net/usb/pegasus.c:746 [inline]
 pegasus_probe+0x10e7/0x4080 drivers/net/usb/pegasus.c:1152
....

Local variable ----data.i@pegasus_probe created at:
 get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
 pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152
 get_interrupt_interval drivers/net/usb/pegasus.c:1151 [inline]
 pegasus_probe+0xe57/0x4080 drivers/net/usb/pegasus.c:1152

Reported-and-tested-by: syzbot+02c9f70f3afae308464a@syzkaller.appspotmail.com
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Pavel Skripkin <paskripkin@gmail.com>
---

Changes in v2:
	Rebased on top of -net

---
 drivers/net/usb/pegasus.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/pegasus.c b/drivers/net/usb/pegasus.c
index f18b03be1..652e9fcf0 100644
--- a/drivers/net/usb/pegasus.c
+++ b/drivers/net/usb/pegasus.c
@@ -744,12 +744,16 @@ static inline void disable_net_traffic(pegasus_t *pegasus)
 	set_registers(pegasus, EthCtrl0, sizeof(tmp), &tmp);
 }
 
-static inline void get_interrupt_interval(pegasus_t *pegasus)
+static inline int get_interrupt_interval(pegasus_t *pegasus)
 {
 	u16 data;
 	u8 interval;
+	int ret;
+
+	ret = read_eprom_word(pegasus, 4, &data);
+	if (ret < 0)
+		return ret;
 
-	read_eprom_word(pegasus, 4, &data);
 	interval = data >> 8;
 	if (pegasus->usb->speed != USB_SPEED_HIGH) {
 		if (interval < 0x80) {
@@ -764,6 +768,8 @@ static inline void get_interrupt_interval(pegasus_t *pegasus)
 		}
 	}
 	pegasus->intr_interval = interval;
+
+	return 0;
 }
 
 static void set_carrier(struct net_device *net)
@@ -1165,7 +1171,9 @@ static int pegasus_probe(struct usb_interface *intf,
 				| NETIF_MSG_PROBE | NETIF_MSG_LINK);
 
 	pegasus->features = usb_dev_id[dev_index].private;
-	get_interrupt_interval(pegasus);
+	res = get_interrupt_interval(pegasus);
+	if (res)
+		goto out2;
 	if (reset_mac(pegasus)) {
 		dev_err(&intf->dev, "can't reset MAC\n");
 		res = -EIO;
-- 
2.32.0

Re: [PATCH net v2] net: pegasus: fix uninit-value in get_interrupt_interval

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (refs/heads/master):

On Wed,  4 Aug 2021 17:30:05 +0300 you wrote:
> Syzbot reported uninit value pegasus_probe(). The problem was in missing
> error handling.
> 
> get_interrupt_interval() internally calls read_eprom_word() which can
> fail in some cases. For example: failed to receive usb control message.
> These cases should be handled to prevent uninit value bug, since
> read_eprom_word() will not initialize passed stack variable in case of
> internal failure.
> 
> [...]

Here is the summary with links:
  - [net,v2] net: pegasus: fix uninit-value in get_interrupt_interval
    https://git.kernel.org/netdev/net/c/af35fc37354c

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html