Re: Duplicate crash reports related with smsc75xx/smsc95xx and root cause analysis

[Greg KH]

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