Date:   Thu, 21 Jan 2021 16:47:37 +0800
From:   慕冬亮 <mudongliangabcd@...il.com>
To:     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, Greg KH <greg@...ah.com>
Cc:     syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
        syzkaller <syzkaller@...glegroups.com>
Subject: "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value
 in rt2500usb_probe_hw" should be duplicate crash reports

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


--
My best regards to you.

     No System Is Safe!
     Dongliang Mu

Powered by blists - more mailing lists