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 [off-list ref] wrote:

On Thu, Jan 21, 2021 at 04:47:37PM +0800, 慕冬亮 wrote:

quoted

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;
------------------------------------------------------------------------------------------------------------------------

quoted

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