* "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports
@ 2021-01-21 8:47 慕冬亮
2021-01-21 8:52 ` Greg KH
2021-01-21 9:25 ` Stanislaw Gruszka
0 siblings, 2 replies; 4+ messages in thread
From: 慕冬亮 @ 2021-01-21 8:47 UTC (permalink / raw)
To: davem, helmut.schaa, kuba, kvalo, linux-kernel, linux-wireless,
netdev, stf_xl, Greg KH
Cc: syzkaller-bugs, syzkaller
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,
®, 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,
®, 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(®, 0, sizeof(reg));
rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ,
USB_VENDOR_REQUEST_IN, offset,
®, sizeof(reg));
@@ -58,6 +59,7 @@ static u16 rt2500usb_register_read_lock(struct
rt2x00_dev *rt2x00dev,
const unsigned int offset)
{
__le16 reg;
+ memset(®, 0, sizeof(reg));
rt2x00usb_vendor_req_buff_lock(rt2x00dev, USB_MULTI_READ,
USB_VENDOR_REQUEST_IN, offset,
®, 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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports
2021-01-21 8:47 "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports 慕冬亮
@ 2021-01-21 8:52 ` Greg KH
2021-01-21 9:24 ` 慕冬亮
2021-01-21 9:25 ` Stanislaw Gruszka
1 sibling, 1 reply; 4+ messages in thread
From: Greg KH @ 2021-01-21 8:52 UTC (permalink / raw)
To: 慕冬亮
Cc: davem, helmut.schaa, kuba, kvalo, linux-kernel, linux-wireless,
netdev, stf_xl, syzkaller-bugs, syzkaller
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,
> ®, 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,
> ®, 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(®, 0, sizeof(reg));
> rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ,
> USB_VENDOR_REQUEST_IN, offset,
> ®, sizeof(reg));
> @@ -58,6 +59,7 @@ static u16 rt2500usb_register_read_lock(struct
> rt2x00_dev *rt2x00dev,
> const unsigned int offset)
> {
> __le16 reg;
> + memset(®, 0, sizeof(reg));
> rt2x00usb_vendor_req_buff_lock(rt2x00dev, USB_MULTI_READ,
> USB_VENDOR_REQUEST_IN, offset,
> ®, 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports
2021-01-21 8:52 ` Greg KH
@ 2021-01-21 9:24 ` 慕冬亮
0 siblings, 0 replies; 4+ messages in thread
From: 慕冬亮 @ 2021-01-21 9:24 UTC (permalink / raw)
To: Greg KH
Cc: davem, helmut.schaa, kuba, kvalo, linux-kernel, linux-wireless,
netdev, stf_xl, syzkaller-bugs, syzkaller
On Thu, Jan 21, 2021 at 4:52 PM Greg KH <gregkh@linuxfoundation.org> 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,
> > ®, 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,
> > ®, 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(®, 0, sizeof(reg));
> > rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ,
> > USB_VENDOR_REQUEST_IN, offset,
> > ®, sizeof(reg));
> > @@ -58,6 +59,7 @@ static u16 rt2500usb_register_read_lock(struct
> > rt2x00_dev *rt2x00dev,
> > const unsigned int offset)
> > {
> > __le16 reg;
> > + memset(®, 0, sizeof(reg));
> > rt2x00usb_vendor_req_buff_lock(rt2x00dev, USB_MULTI_READ,
> > USB_VENDOR_REQUEST_IN, offset,
> > ®, 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports
2021-01-21 8:47 "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports 慕冬亮
2021-01-21 8:52 ` Greg KH
@ 2021-01-21 9:25 ` Stanislaw Gruszka
1 sibling, 0 replies; 4+ messages in thread
From: Stanislaw Gruszka @ 2021-01-21 9:25 UTC (permalink / raw)
To: 慕冬亮
Cc: davem, helmut.schaa, kuba, kvalo, linux-kernel, linux-wireless,
netdev, Greg KH, syzkaller-bugs, syzkaller
On Thu, Jan 21, 2021 at 04:47:37PM +0800, 慕冬亮 wrote:
> ## 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(®, 0, sizeof(reg));
Simpler would be just to initialize like this: __le16 reg = 0;
Stanislaw
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-21 19:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-21 8:47 "KMSAN: uninit-value in rt2500usb_bbp_read" and "KMSAN: uninit-value in rt2500usb_probe_hw" should be duplicate crash reports 慕冬亮
2021-01-21 8:52 ` Greg KH
2021-01-21 9:24 ` 慕冬亮
2021-01-21 9:25 ` Stanislaw Gruszka
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).