| Message ID | 20210730214411.1973-1-paskripkin@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | net: pegasus: fix uninit-value in get_interrupt_interval | expand |
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 > >
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
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
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, ®di); + 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)
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, ®di); > + 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
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
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
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].
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;
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(-)