diff mbox series

"KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports

Message ID CAD-N9QX=vVdiSf5UkuoYovamfw5a0e5RQJA0dQMOKmCbs-Gyiw@mail.gmail.com (mailing list archive)
State Not Applicable
Headers show
Series "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Dongliang Mu Jan. 21, 2021, 8:47 a.m. UTC 
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.

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

Comments

Greg KH Jan. 21, 2021, 8:52 a.m. UTC | #1 
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
diff mbox series

Patch

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,