Message-ID: <YAlAy/tQXW0X310V@kroah.com>
Date:   Thu, 21 Jan 2021 09:52:27 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     慕冬亮 <mudongliangabcd@...il.com>
Cc:     davem@...emloft.net, helmut.schaa@...glemail.com, kuba@...nel.org,
        kvalo@...eaurora.org, linux-kernel <linux-kernel@...r.kernel.org>,
        linux-wireless@...r.kernel.org, netdev@...r.kernel.org,
        stf_xl@...pl, syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: Re: "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN:
 uninit-value in rt2500usb_probe_hw" should be duplicate crash reports

On Thu, Jan 21, 2021 at 04:47:37PM +0800, 慕冬亮 wrote:
> Dear kernel developers,
> 
> I found that on the syzbot dashboard, “KMSAN: uninit-value in
> rt2500usb_bbp_read” [1] and "KMSAN: uninit-value in
> rt2500usb_probe_hw" [2] should share the same root cause.
> 
> ## Duplication
> 
> The reasons for the above statement:
> 1) The PoCs are exactly the same with each other;
> 2) The stack trace is almost the same except for the top 2 functions;
> 
> ## Root Cause Analysis
> 
> After looking at the difference between the two stack traces, we found
> they diverge at the function - rt2500usb_probe_hw.
> ------------------------------------------------------------------------------------------------------------------------
> static int rt2500usb_probe_hw(struct rt2x00_dev *rt2x00dev)
> {
>         ......
>         // rt2500usb_validate_eeprom->rt2500usb_bbp_read->rt2500usb_regbusy_read->rt2500usb_register_read_lock
> from KMSAN
>         retval = rt2500usb_validate_eeprom(rt2x00dev);
>         if (retval)
>                 return retval;
>         // rt2500usb_init_eeprom-> rt2500usb_register_read from KMSAN
>         retval = rt2500usb_init_eeprom(rt2x00dev);
>         if (retval)
>                 return retval;
> ------------------------------------------------------------------------------------------------------------------------
> >From the implementation of rt2500usb_register_read and
> rt2500usb_register_read_lock, we know that, in some situation, reg is
> not initialized in the function invocation
> (rt2x00usb_vendor_request_buff/rt2x00usb_vendor_req_buff_lock), and
> KMSAN reports uninit-value at its first memory access.
> ------------------------------------------------------------------------------------------------------------------------
> static u16 rt2500usb_register_read(struct rt2x00_dev *rt2x00dev,
>                                    const unsigned int offset)
> {
>         __le16 reg;
>         // reg is not initialized during the following function all
>         rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ,
>                                       USB_VENDOR_REQUEST_IN, offset,
>                                       &reg, sizeof(reg));
>         return le16_to_cpu(reg);
> }
> static u16 rt2500usb_register_read_lock(struct rt2x00_dev *rt2x00dev,
>                                         const unsigned int offset)
> {
>         __le16 reg;
>         // reg is not initialized during the following function all
>         rt2x00usb_vendor_req_buff_lock(rt2x00dev, USB_MULTI_READ,
>                                        USB_VENDOR_REQUEST_IN, offset,
>                                        &reg, sizeof(reg), REGISTER_TIMEOUT);
>         return le16_to_cpu(reg);
> }
> ------------------------------------------------------------------------------------------------------------------------
> Take rt2x00usb_vendor_req_buff_lock as an example, let me illustrate
> the issue when the "reg" variable is uninitialized. No matter the CSR
> cache is unavailable or the status is not right, the buffer or reg
> will be not initialized.
> And all those issues are probabilistic events. If they occur in
> rt2500usb_register_read, KMSAN reports "uninit-value in
> rt2500usb_probe_hw"; Otherwise, it reports "uninit-value in
> rt2500usb_bbp_read".
> ------------------------------------------------------------------------------------------------------------------------
> int rt2x00usb_vendor_req_buff_lock(struct rt2x00_dev *rt2x00dev,
>                                    const u8 request, const u8 requesttype,
>                                    const u16 offset, void *buffer,
>                                    const u16 buffer_length, const int timeout)
> {
>         if (unlikely(!rt2x00dev->csr.cache || buffer_length > CSR_CACHE_SIZE)) {
>                 rt2x00_err(rt2x00dev, "CSR cache not available\n");
>                 return -ENOMEM;
>         }
> 
>         if (requesttype == USB_VENDOR_REQUEST_OUT)
>                 memcpy(rt2x00dev->csr.cache, buffer, buffer_length);
> 
>         status = rt2x00usb_vendor_request(rt2x00dev, request, requesttype,
>                                           offset, 0, rt2x00dev->csr.cache,
>                                           buffer_length, timeout);
> 
>         if (!status && requesttype == USB_VENDOR_REQUEST_IN)
>                 memcpy(buffer, rt2x00dev->csr.cache, buffer_length);
> 
>         return status;
> }
> ------------------------------------------------------------------------------------------------------------------------
> 
> ## Patch
> 
> I propose to memset reg variable before invoking
> rt2x00usb_vendor_req_buff_lock/rt2x00usb_vendor_request_buff.
> 
> ------------------------------------------------------------------------------------------------------------------------
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2500usb.c
> b/drivers/net/wireless/ralink/rt2x00/rt2500usb.c
> index fce05fc88aaf..f6c93a25b18c 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2500usb.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2500usb.c
> @@ -48,6 +48,7 @@ static u16 rt2500usb_register_read(struct rt2x00_dev
> *rt2x00dev,
>                                    const unsigned int offset)
>  {
>         __le16 reg;
> +       memset(&reg, 0, sizeof(reg));
>         rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ,
>                                       USB_VENDOR_REQUEST_IN, offset,
>                                       &reg, sizeof(reg));
> @@ -58,6 +59,7 @@ static u16 rt2500usb_register_read_lock(struct
> rt2x00_dev *rt2x00dev,
>                                         const unsigned int offset)
>  {
>         __le16 reg;
> +       memset(&reg, 0, sizeof(reg));
>         rt2x00usb_vendor_req_buff_lock(rt2x00dev, USB_MULTI_READ,
>                                        USB_VENDOR_REQUEST_IN, offset,
>                                        &reg, sizeof(reg), REGISTER_TIMEOUT);
> ------------------------------------------------------------------------------------------------------------------------
> 
> 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 rt2500usb_bbp_read” -
> https://syzkaller.appspot.com/bug?id=f35d123de7d393019c1ed4d4e60dc66596ed62cd
> [2] “KMSAN: uninit-value in rt2500usb_probe_hw” -
> https://syzkaller.appspot.com/bug?id=5402df7259c74e15a12992e739b5ac54c9b8a4ce
> 

Can you please resend this in a form in which we can apply it?  Full
details on how to do this can be found in
Documentation/SubmittingPatches.

thanks,

greg k-h

Powered by blists - more mailing lists