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 4:52 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> 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.

I have sent a patch to the corresponding maintainers. We can take it
as a base to discuss the corresponding bug.

>
> thanks,
>
> greg k-h