| Message ID | CAD-N9QUdXFhTqZXpjg02Ya7viR8WmkORbU7pwNTquNg8k_kzMg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Duplicate crash reports related with smsc75xx/smsc95xx and root cause analysis | expand |
On Sat, Jan 23, 2021 at 1:40 PM 慕冬亮 <mudongliangabcd@gmail.com> wrote: > > Dear kernel developers, > > I found that on the syzbot dashboard, “KMSAN: uninit-value in > smsc75xx_read_eeprom (2)” [1], > "KMSAN: uninit-value in smsc95xx_read_eeprom (2)" [2], "KMSAN: > uninit-value in smsc75xx_bind" [3], > "KMSAN: uninit-value in smsc95xx_reset" [4], "KMSAN: uninit-value in > smsc95xx_wait_eeprom (2)" [5] > should share the same root cause. > > ## Root Cause Analysis && Different behaviors > > The root cause of these crash reports resides in the > "__smsc75xx_read_reg/__smsc95xx_read_reg". Take __smsc95xx_read_reg as > an example, > > ----------------------------------------------------------------------------------------------------------------- > static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > u32 buf; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > BUG_ON(!dev); > > if (!in_pm) > fn = usbnet_read_cmd; > else > fn = usbnet_read_cmd_nopm; > > ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN > | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > 0, index, &buf, 4); > if (unlikely(ret < 0)) { > netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n", > index, ret); > return ret; > } > > le32_to_cpus(&buf); > *data = buf; > > return ret; > } > > > static int __must_check smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev) > { > unsigned long start_time = jiffies; > u32 val; > int ret; > > do { > ret = smsc95xx_read_reg(dev, E2P_CMD, &val); > if (ret < 0) { > netdev_warn(dev->net, "Error reading E2P_CMD\n"); > return ret; > } > > if (!(val & E2P_CMD_BUSY_)) > return 0; > ...... > } > ----------------------------------------------------------------------------------------------------------------- > > In a special situation, local variable "buf" is not initialized with > "fn" function invocation. And the ret is bigger than zero, and buf is > assigned to "*data". In its parent function - > smsc95xx_eeprom_confirm_not_busy, KMSAN reports "uninit-value" when > accessing variable "val". > Note, due to the lack of testing environment, I don't know the > concrete reason for the uninitialization of "buf" local variable. > > The reason for such different crash behaviors is that the event - > "buf" is not initialized is random when > "__smsc75xx_read_reg/__smsc95xx_read_reg" is invoked. > > ## Patch > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > index 4353b370249f..a8e500d92285 100644 > --- a/drivers/net/usb/smsc75xx.c > +++ b/drivers/net/usb/smsc75xx.c > @@ -76,7 +76,7 @@ static int smsc75xx_phy_gig_workaround(struct usbnet *dev); > static int __must_check __smsc75xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > - u32 buf; > + u32 buf = 0; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index 4c8ee1cff4d4..dae3be723e0c 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -70,7 +70,7 @@ MODULE_PARM_DESC(turbo_mode, "Enable multiple frames > per Rx transaction"); > static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > - u32 buf; > + u32 buf = 0; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > If you can have any issues with this statement or our information is > useful to you, please let us know. Thanks very much. > > [1] “KMSAN: uninit-value in smsc75xx_read_eeprom (2)” - url > [2] “KMSAN: uninit-value in smsc95xx_read_eeprom (2)” - URL > [3] "KMSAN: uninit-value in smsc75xx_bind" - > [4] "KMSAN: uninit-value in smsc95xx_reset" - > [5] "KMSAN: uninit-value in smsc95xx_wait_eeprom (2)" - Add links for all five bug reports: [1] “KMSAN: uninit-value in smsc75xx_read_eeprom (2)” - https://syzkaller.appspot.com/bug?id=2fb4e465ed593338d043227e7617cbdfaa03ba01 [2] “KMSAN: uninit-value in smsc95xx_read_eeprom (2)” - https://syzkaller.appspot.com/bug?id=0629febb76ae17ff78874aa68991e542506b1351 [3] "KMSAN: uninit-value in smsc75xx_bind" - https://syzkaller.appspot.com/bug?id=45ee70ca00699d61239bbf9ebc790e33f83add6a [4] "KMSAN: uninit-value in smsc95xx_reset" - https://syzkaller.appspot.com/bug?id=de07a0d125f8f1716eacb7e2ee4ceca251b21511 [5] "KMSAN: uninit-value in smsc95xx_wait_eeprom (2)" - https://syzkaller.appspot.com/bug?id=b4eb76261b208b986ad7683e686c6be4200a7109 > > -- > My best regards to you. > > No System Is Safe! > Dongliang Mu
On Sat, Jan 23, 2021 at 01:40:30PM +0800, 慕冬亮 wrote: > Dear kernel developers, > > I found that on the syzbot dashboard, “KMSAN: uninit-value in > smsc75xx_read_eeprom (2)” [1], > "KMSAN: uninit-value in smsc95xx_read_eeprom (2)" [2], "KMSAN: > uninit-value in smsc75xx_bind" [3], > "KMSAN: uninit-value in smsc95xx_reset" [4], "KMSAN: uninit-value in > smsc95xx_wait_eeprom (2)" [5] > should share the same root cause. > > ## Root Cause Analysis && Different behaviors > > The root cause of these crash reports resides in the > "__smsc75xx_read_reg/__smsc95xx_read_reg". Take __smsc95xx_read_reg as > an example, > > ----------------------------------------------------------------------------------------------------------------- > static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > u32 buf; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > BUG_ON(!dev); > > if (!in_pm) > fn = usbnet_read_cmd; > else > fn = usbnet_read_cmd_nopm; > > ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN > | USB_TYPE_VENDOR | USB_RECIP_DEVICE, > 0, index, &buf, 4); > if (unlikely(ret < 0)) { > netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n", > index, ret); > return ret; > } > > le32_to_cpus(&buf); > *data = buf; > > return ret; > } > > > static int __must_check smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev) > { > unsigned long start_time = jiffies; > u32 val; > int ret; > > do { > ret = smsc95xx_read_reg(dev, E2P_CMD, &val); > if (ret < 0) { > netdev_warn(dev->net, "Error reading E2P_CMD\n"); > return ret; > } > > if (!(val & E2P_CMD_BUSY_)) > return 0; > ...... > } > ----------------------------------------------------------------------------------------------------------------- > > In a special situation, local variable "buf" is not initialized with > "fn" function invocation. And the ret is bigger than zero, and buf is > assigned to "*data". In its parent function - > smsc95xx_eeprom_confirm_not_busy, KMSAN reports "uninit-value" when > accessing variable "val". > Note, due to the lack of testing environment, I don't know the > concrete reason for the uninitialization of "buf" local variable. > > The reason for such different crash behaviors is that the event - > "buf" is not initialized is random when > "__smsc75xx_read_reg/__smsc95xx_read_reg" is invoked. > > ## Patch > > diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c > index 4353b370249f..a8e500d92285 100644 > --- a/drivers/net/usb/smsc75xx.c > +++ b/drivers/net/usb/smsc75xx.c > @@ -76,7 +76,7 @@ static int smsc75xx_phy_gig_workaround(struct usbnet *dev); > static int __must_check __smsc75xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > - u32 buf; > + u32 buf = 0; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c > index 4c8ee1cff4d4..dae3be723e0c 100644 > --- a/drivers/net/usb/smsc95xx.c > +++ b/drivers/net/usb/smsc95xx.c > @@ -70,7 +70,7 @@ MODULE_PARM_DESC(turbo_mode, "Enable multiple frames > per Rx transaction"); > static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, > u32 *data, int in_pm) > { > - u32 buf; > + u32 buf = 0; > int ret; > int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); > > If you can have any issues with this statement or our information is > useful to you, please let us know. Thanks very much. > > [1] “KMSAN: uninit-value in smsc75xx_read_eeprom (2)” - url > [2] “KMSAN: uninit-value in smsc95xx_read_eeprom (2)” - URL > [3] "KMSAN: uninit-value in smsc75xx_bind" - > [4] "KMSAN: uninit-value in smsc95xx_reset" - > [5] "KMSAN: uninit-value in smsc95xx_wait_eeprom (2)" - As I asked before, please just turn this into a real patch and submit it to the syzbot to see if it fixes the issue. If so, then submit it normally, no need to do any huge writeup. thnaks, greg k-h
diff --git a/drivers/net/usb/smsc75xx.c b/drivers/net/usb/smsc75xx.c index 4353b370249f..a8e500d92285 100644 --- a/drivers/net/usb/smsc75xx.c +++ b/drivers/net/usb/smsc75xx.c @@ -76,7 +76,7 @@ static int smsc75xx_phy_gig_workaround(struct usbnet *dev); static int __must_check __smsc75xx_read_reg(struct usbnet *dev, u32 index, u32 *data, int in_pm) { - u32 buf; + u32 buf = 0; int ret; int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c index 4c8ee1cff4d4..dae3be723e0c 100644 --- a/drivers/net/usb/smsc95xx.c +++ b/drivers/net/usb/smsc95xx.c @@ -70,7 +70,7 @@ MODULE_PARM_DESC(turbo_mode, "Enable multiple frames per Rx transaction"); static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data, int in_pm) { - u32 buf; + u32 buf = 0; int ret; int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
Dear kernel developers, I found that on the syzbot dashboard, “KMSAN: uninit-value in smsc75xx_read_eeprom (2)” [1], "KMSAN: uninit-value in smsc95xx_read_eeprom (2)" [2], "KMSAN: uninit-value in smsc75xx_bind" [3], "KMSAN: uninit-value in smsc95xx_reset" [4], "KMSAN: uninit-value in smsc95xx_wait_eeprom (2)" [5] should share the same root cause. ## Root Cause Analysis && Different behaviors The root cause of these crash reports resides in the "__smsc75xx_read_reg/__smsc95xx_read_reg". Take __smsc95xx_read_reg as an example, ----------------------------------------------------------------------------------------------------------------- static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index, u32 *data, int in_pm) { u32 buf; int ret; int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16); BUG_ON(!dev); if (!in_pm) fn = usbnet_read_cmd; else fn = usbnet_read_cmd_nopm; ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE, 0, index, &buf, 4); if (unlikely(ret < 0)) { netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n", index, ret); return ret; } le32_to_cpus(&buf); *data = buf; return ret; } static int __must_check smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev) { unsigned long start_time = jiffies; u32 val; int ret; do { ret = smsc95xx_read_reg(dev, E2P_CMD, &val); if (ret < 0) { netdev_warn(dev->net, "Error reading E2P_CMD\n"); return ret; } if (!(val & E2P_CMD_BUSY_)) return 0; ...... } ----------------------------------------------------------------------------------------------------------------- In a special situation, local variable "buf" is not initialized with "fn" function invocation. And the ret is bigger than zero, and buf is assigned to "*data". In its parent function - smsc95xx_eeprom_confirm_not_busy, KMSAN reports "uninit-value" when accessing variable "val". Note, due to the lack of testing environment, I don't know the concrete reason for the uninitialization of "buf" local variable. The reason for such different crash behaviors is that the event - "buf" is not initialized is random when "__smsc75xx_read_reg/__smsc95xx_read_reg" is invoked. ## Patch If you can have any issues with this statement or our information is useful to you, please let us know. Thanks very much. [1] “KMSAN: uninit-value in smsc75xx_read_eeprom (2)” - url [2] “KMSAN: uninit-value in smsc95xx_read_eeprom (2)” - URL [3] "KMSAN: uninit-value in smsc75xx_bind" - [4] "KMSAN: uninit-value in smsc95xx_reset" - [5] "KMSAN: uninit-value in smsc95xx_wait_eeprom (2)" - -- My best regards to you. No System Is Safe! Dongliang Mu