| Message ID | 20210121092026.3261412-1-mudongliangabcd@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | rt2x00: reset reg earlier in rt2500usb_register_read | expand |
On Thu, Jan 21, 2021 at 05:20:26PM +0800, Dongliang Mu wrote: > In the function rt2500usb_register_read(_lock), reg is uninitialized > in some situation. Then KMSAN reports uninit-value at its first memory > access. To fix this issue, add one reg initialization in the function > rt2500usb_register_read and rt2500usb_register_read_lock > > BUG: KMSAN: uninit-value in rt2500usb_init_eeprom rt2500usb.c:1443 [inline] > BUG: KMSAN: uninit-value in rt2500usb_probe_hw+0xb5e/0x22a0 rt2500usb.c:1757 > CPU: 0 PID: 3369 Comm: kworker/0:2 Not tainted 5.3.0-rc7+ #0 > Hardware name: Google Compute Engine > Workqueue: usb_hub_wq hub_event > Call Trace: > __dump_stack lib/dump_stack.c:77 [inline] > dump_stack+0x191/0x1f0 lib/dump_stack.c:113 > kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109 > __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294 > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1443 [inline] > rt2500usb_probe_hw+0xb5e/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > rt2x00usb_probe+0x7ae/0xf60 wireless/ralink/rt2x00/rt2x00usb.c:842 > rt2500usb_probe+0x50/0x60 wireless/ralink/rt2x00/rt2500usb.c:1966 > ...... > > Local variable description: ----reg.i.i@rt2500usb_probe_hw > Variable was created at: > rt2500usb_register_read wireless/ralink/rt2x00/rt2500usb.c:51 [inline] > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1440 [inline] > rt2500usb_probe_hw+0x774/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > --- > drivers/net/wireless/ralink/rt2x00/rt2500usb.c | 2 ++ > 1 file changed, 2 insertions(+) > > 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)); As was pointed out, just set reg = 0 on the line above please. > rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ, > USB_VENDOR_REQUEST_IN, offset, > ®, sizeof(reg)); Are you sure this is valid to call this function with a variable on the stack like this? How did you test this change? thanks, greg k-h
On Thu, Jan 21, 2021 at 5:49 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jan 21, 2021 at 05:20:26PM +0800, Dongliang Mu wrote: > > In the function rt2500usb_register_read(_lock), reg is uninitialized > > in some situation. Then KMSAN reports uninit-value at its first memory > > access. To fix this issue, add one reg initialization in the function > > rt2500usb_register_read and rt2500usb_register_read_lock > > > > BUG: KMSAN: uninit-value in rt2500usb_init_eeprom rt2500usb.c:1443 [inline] > > BUG: KMSAN: uninit-value in rt2500usb_probe_hw+0xb5e/0x22a0 rt2500usb.c:1757 > > CPU: 0 PID: 3369 Comm: kworker/0:2 Not tainted 5.3.0-rc7+ #0 > > Hardware name: Google Compute Engine > > Workqueue: usb_hub_wq hub_event > > Call Trace: > > __dump_stack lib/dump_stack.c:77 [inline] > > dump_stack+0x191/0x1f0 lib/dump_stack.c:113 > > kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109 > > __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294 > > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1443 [inline] > > rt2500usb_probe_hw+0xb5e/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > > rt2x00usb_probe+0x7ae/0xf60 wireless/ralink/rt2x00/rt2x00usb.c:842 > > rt2500usb_probe+0x50/0x60 wireless/ralink/rt2x00/rt2500usb.c:1966 > > ...... > > > > Local variable description: ----reg.i.i@rt2500usb_probe_hw > > Variable was created at: > > rt2500usb_register_read wireless/ralink/rt2x00/rt2500usb.c:51 [inline] > > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1440 [inline] > > rt2500usb_probe_hw+0x774/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > --- > > drivers/net/wireless/ralink/rt2x00/rt2500usb.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > 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)); > > As was pointed out, just set reg = 0 on the line above please. I've sent another patch. BTW, I set "--subject-prefix="PATCH v2" in my git-send-mail command. But it does not show "v2" in the subject of the new email. > > > rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ, > > USB_VENDOR_REQUEST_IN, offset, > > ®, sizeof(reg)); > > Are you sure this is valid to call this function with a variable on the > stack like this? How did you test this change? First, I did not do any changes to this call. Second, the programming style to pass the pointer of stack variable as arguments is not really good. Third, I check this same code file, there are many code snippets with such programming style. :( > > thanks, > > greg k-h
On Thu, Jan 21, 2021 at 06:59:08PM +0800, 慕冬亮 wrote: > On Thu, Jan 21, 2021 at 5:49 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > > > On Thu, Jan 21, 2021 at 05:20:26PM +0800, Dongliang Mu wrote: > > > In the function rt2500usb_register_read(_lock), reg is uninitialized > > > in some situation. Then KMSAN reports uninit-value at its first memory > > > access. To fix this issue, add one reg initialization in the function > > > rt2500usb_register_read and rt2500usb_register_read_lock > > > > > > BUG: KMSAN: uninit-value in rt2500usb_init_eeprom rt2500usb.c:1443 [inline] > > > BUG: KMSAN: uninit-value in rt2500usb_probe_hw+0xb5e/0x22a0 rt2500usb.c:1757 > > > CPU: 0 PID: 3369 Comm: kworker/0:2 Not tainted 5.3.0-rc7+ #0 > > > Hardware name: Google Compute Engine > > > Workqueue: usb_hub_wq hub_event > > > Call Trace: > > > __dump_stack lib/dump_stack.c:77 [inline] > > > dump_stack+0x191/0x1f0 lib/dump_stack.c:113 > > > kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109 > > > __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294 > > > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1443 [inline] > > > rt2500usb_probe_hw+0xb5e/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > > > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > > > rt2x00usb_probe+0x7ae/0xf60 wireless/ralink/rt2x00/rt2x00usb.c:842 > > > rt2500usb_probe+0x50/0x60 wireless/ralink/rt2x00/rt2500usb.c:1966 > > > ...... > > > > > > Local variable description: ----reg.i.i@rt2500usb_probe_hw > > > Variable was created at: > > > rt2500usb_register_read wireless/ralink/rt2x00/rt2500usb.c:51 [inline] > > > rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1440 [inline] > > > rt2500usb_probe_hw+0x774/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 > > > rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 > > > > > > Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> > > > --- > > > drivers/net/wireless/ralink/rt2x00/rt2500usb.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > 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)); > > > > As was pointed out, just set reg = 0 on the line above please. > > I've sent another patch. > > BTW, I set "--subject-prefix="PATCH v2" in my git-send-mail command. > But it does not show "v2" in the subject of the new email. You can set the "v2" in the 'git format-patch' option, right? thanks, greg k-h
On Thu, Jan 21, 2021 at 06:59:08PM +0800, 慕冬亮 wrote: > > > rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ, > > > USB_VENDOR_REQUEST_IN, offset, > > > ®, sizeof(reg)); > > > > Are you sure this is valid to call this function with a variable on the > > stack like this? How did you test this change? > > First, I did not do any changes to this call. Second, the programming > style to pass the pointer of stack variable as arguments is not really > good. Third, I check this same code file, there are many code snippets > with such programming style. :( I know you did not change it, what I am asking is how did you test this change works? I think the kernel will warn you in huge ways that using this pointer on the stack is incorrect, which implies you did not test this change :( thanks, greg k-h
On Thu, Jan 21, 2021 at 7:21 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Jan 21, 2021 at 06:59:08PM +0800, 慕冬亮 wrote: > > > > rt2x00usb_vendor_request_buff(rt2x00dev, USB_MULTI_READ, > > > > USB_VENDOR_REQUEST_IN, offset, > > > > ®, sizeof(reg)); > > > > > > Are you sure this is valid to call this function with a variable on the > > > stack like this? How did you test this change? > > > > First, I did not do any changes to this call. Second, the programming > > style to pass the pointer of stack variable as arguments is not really > > good. Third, I check this same code file, there are many code snippets > > with such programming style. :( > > I know you did not change it, what I am asking is how did you test this > change works? I think the kernel will warn you in huge ways that using > this pointer on the stack is incorrect, which implies you did not test > this change :( > I tested this patch only with PoC. The patched kernel version did not crash when executing the PoC. BTW, I did not take notice of the warning information as there are many many warnings in building KMSAN. > thanks, > > greg k-h
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);
In the function rt2500usb_register_read(_lock), reg is uninitialized in some situation. Then KMSAN reports uninit-value at its first memory access. To fix this issue, add one reg initialization in the function rt2500usb_register_read and rt2500usb_register_read_lock BUG: KMSAN: uninit-value in rt2500usb_init_eeprom rt2500usb.c:1443 [inline] BUG: KMSAN: uninit-value in rt2500usb_probe_hw+0xb5e/0x22a0 rt2500usb.c:1757 CPU: 0 PID: 3369 Comm: kworker/0:2 Not tainted 5.3.0-rc7+ #0 Hardware name: Google Compute Engine Workqueue: usb_hub_wq hub_event Call Trace: __dump_stack lib/dump_stack.c:77 [inline] dump_stack+0x191/0x1f0 lib/dump_stack.c:113 kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109 __msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294 rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1443 [inline] rt2500usb_probe_hw+0xb5e/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 rt2x00usb_probe+0x7ae/0xf60 wireless/ralink/rt2x00/rt2x00usb.c:842 rt2500usb_probe+0x50/0x60 wireless/ralink/rt2x00/rt2500usb.c:1966 ...... Local variable description: ----reg.i.i@rt2500usb_probe_hw Variable was created at: rt2500usb_register_read wireless/ralink/rt2x00/rt2500usb.c:51 [inline] rt2500usb_init_eeprom wireless/ralink/rt2x00/rt2500usb.c:1440 [inline] rt2500usb_probe_hw+0x774/0x22a0 wireless/ralink/rt2x00/rt2500usb.c:1757 rt2x00lib_probe_dev+0xba9/0x3260 wireless/ralink/rt2x00/rt2x00dev.c:1427 Signed-off-by: Dongliang Mu <mudongliangabcd@gmail.com> --- drivers/net/wireless/ralink/rt2x00/rt2500usb.c | 2 ++ 1 file changed, 2 insertions(+)