* [PATCH v2 0/2] staging: rtl8712: fix uninit-value 'data' and 'mac' @ 2022-05-06 3:13 Wang Cheng 2022-05-06 3:15 ` [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends Wang Cheng 2022-05-06 3:16 ` [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() Wang Cheng 0 siblings, 2 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 3:13 UTC (permalink / raw) To: dan.carpenter, paskripkin; +Cc: linux-staging, linux-kernel This is a v2 patch to fix KMSAN: uninit-value in r871xu_drv_init, https://syzkaller.appspot.com/bug?id=3cd92b1d85428b128503bfa7a250294c9ae00bd8 Previous version and discussion could be seen here: https://lore.kernel.org/all/20220414141223.qwiznrwgjyywngfg@ppc.localdomain/ Changelog v1->v2: - Split to two patches. - Add kmsan reports. - Fix uninit-value 'data' by adding error checking and initialization rather than changing logic of r8712_usbctrl_vendorreq(). Wang Cheng (2): staging: rtl8712: fix uninit-value in usb_read8() and friends staging: rtl8712: fix uninit-value in r871xu_drv_init() drivers/staging/rtl8712/usb_intf.c | 6 +++--- drivers/staging/rtl8712/usb_ops.c | 27 ++++++++++++++++++--------- 2 files changed, 21 insertions(+), 12 deletions(-) -- 2.33.1 ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends 2022-05-06 3:13 [PATCH v2 0/2] staging: rtl8712: fix uninit-value 'data' and 'mac' Wang Cheng @ 2022-05-06 3:15 ` Wang Cheng 2022-05-06 7:10 ` Dan Carpenter 2022-05-06 7:23 ` Pavel Skripkin 2022-05-06 3:16 ` [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() Wang Cheng 1 sibling, 2 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 3:15 UTC (permalink / raw) To: dan.carpenter, paskripkin; +Cc: linux-staging, linux-kernel When r8712_usbctrl_vendorreq() returns negative, 'data' in usb_read{8,16,32} will not be initialized. BUG: KMSAN: uninit-value in string_nocheck lib/vsprintf.c:643 [inline] BUG: KMSAN: uninit-value in string+0x4ec/0x6f0 lib/vsprintf.c:725 string_nocheck lib/vsprintf.c:643 [inline] string+0x4ec/0x6f0 lib/vsprintf.c:725 vsnprintf+0x2222/0x3650 lib/vsprintf.c:2806 va_format lib/vsprintf.c:1704 [inline] pointer+0x18e6/0x1f70 lib/vsprintf.c:2443 vsnprintf+0x1a9b/0x3650 lib/vsprintf.c:2810 vprintk_store+0x537/0x2150 kernel/printk/printk.c:2158 vprintk_emit+0x28b/0xab0 kernel/printk/printk.c:2256 dev_vprintk_emit+0x5ef/0x6d0 drivers/base/core.c:4604 dev_printk_emit+0x1dd/0x21f drivers/base/core.c:4615 __dev_printk+0x3be/0x440 drivers/base/core.c:4627 _dev_info+0x1ea/0x22f drivers/base/core.c:4673 r871xu_drv_init+0x1929/0x3070 drivers/staging/rtl8712/usb_intf.c:401 usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396 really_probe+0x6c7/0x1350 drivers/base/dd.c:621 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:752 driver_probe_device drivers/base/dd.c:782 [inline] __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:899 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427 __device_attach+0x593/0x8e0 drivers/base/dd.c:970 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1017 bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487 device_add+0x1fff/0x26e0 drivers/base/core.c:3405 usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170 usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238 usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293 really_probe+0x6c7/0x1350 drivers/base/dd.c:621 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:752 driver_probe_device drivers/base/dd.c:782 [inline] __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:899 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427 __device_attach+0x593/0x8e0 drivers/base/dd.c:970 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1017 bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487 device_add+0x1fff/0x26e0 drivers/base/core.c:3405 usb_new_device+0x1b91/0x2950 drivers/usb/core/hub.c:2566 hub_port_connect drivers/usb/core/hub.c:5363 [inline] hub_port_connect_change drivers/usb/core/hub.c:5507 [inline] port_event drivers/usb/core/hub.c:5665 [inline] hub_event+0x58e3/0x89e0 drivers/usb/core/hub.c:5747 process_one_work+0xdb6/0x1820 kernel/workqueue.c:2289 worker_thread+0x10d0/0x2240 kernel/workqueue.c:2436 kthread+0x3c7/0x500 kernel/kthread.c:376 ret_from_fork+0x1f/0x30 Local variable data created at: usb_read8+0x5d/0x130 drivers/staging/rtl8712/usb_ops.c:33 r8712_read8+0xa5/0xd0 drivers/staging/rtl8712/rtl8712_io.c:29 Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com Signed-off-by: Wang Cheng <wanngchenng@gmail.com> --- drivers/staging/rtl8712/usb_ops.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-) diff --git a/drivers/staging/rtl8712/usb_ops.c b/drivers/staging/rtl8712/usb_ops.c index e64845e6adf3..af9966d03979 100644 --- a/drivers/staging/rtl8712/usb_ops.c +++ b/drivers/staging/rtl8712/usb_ops.c @@ -29,7 +29,8 @@ static u8 usb_read8(struct intf_hdl *intfhdl, u32 addr) u16 wvalue; u16 index; u16 len; - __le32 data; + int status; + __le32 data = 0; struct intf_priv *intfpriv = intfhdl->pintfpriv; request = 0x05; @@ -37,8 +38,10 @@ static u8 usb_read8(struct intf_hdl *intfhdl, u32 addr) index = 0; wvalue = (u16)(addr & 0x0000ffff); len = 1; - r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, &data, len, - requesttype); + status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, + &data, len, requesttype); + if (status < 0) + return 0; return (u8)(le32_to_cpu(data) & 0x0ff); } @@ -49,7 +52,8 @@ static u16 usb_read16(struct intf_hdl *intfhdl, u32 addr) u16 wvalue; u16 index; u16 len; - __le32 data; + int status; + __le32 data = 0; struct intf_priv *intfpriv = intfhdl->pintfpriv; request = 0x05; @@ -57,8 +61,10 @@ static u16 usb_read16(struct intf_hdl *intfhdl, u32 addr) index = 0; wvalue = (u16)(addr & 0x0000ffff); len = 2; - r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, &data, len, - requesttype); + status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, + &data, len, requesttype); + if (status < 0) + return 0; return (u16)(le32_to_cpu(data) & 0xffff); } @@ -69,7 +75,8 @@ static u32 usb_read32(struct intf_hdl *intfhdl, u32 addr) u16 wvalue; u16 index; u16 len; - __le32 data; + int status; + __le32 data = 0; struct intf_priv *intfpriv = intfhdl->pintfpriv; request = 0x05; @@ -77,8 +84,10 @@ static u32 usb_read32(struct intf_hdl *intfhdl, u32 addr) index = 0; wvalue = (u16)(addr & 0x0000ffff); len = 4; - r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, &data, len, - requesttype); + status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, + &data, len, requesttype); + if (status < 0) + return 0; return le32_to_cpu(data); } -- 2.33.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends 2022-05-06 3:15 ` [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends Wang Cheng @ 2022-05-06 7:10 ` Dan Carpenter 2022-05-06 11:22 ` Wang Cheng 2022-05-06 7:23 ` Pavel Skripkin 1 sibling, 1 reply; 12+ messages in thread From: Dan Carpenter @ 2022-05-06 7:10 UTC (permalink / raw) To: Wang Cheng; +Cc: paskripkin, linux-staging, linux-kernel Setting "data = 0" will silence the KMSAN warnings but it doesn't fix the bug which is that r8712_usbctrl_vendorreq() treats partial reads as success. The usb_control_msg() returns negatives on total failure and it returns small positives on partial failure. So take the code that I gave you before and put that into r8712_usbctrl_vendorreq(). That's patch 1. These patches become 2 and 3. status = usb_control_msg(); if (status < 0) goto free; if (status != len) { status = -EREMOTEIO; goto free; } if (requesttype == 0x01) memcpy(pdata, pIo_buf, status); regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends 2022-05-06 7:10 ` Dan Carpenter @ 2022-05-06 11:22 ` Wang Cheng 0 siblings, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 11:22 UTC (permalink / raw) To: Dan Carpenter; +Cc: paskripkin, linux-staging, linux-kernel On 22/05/06 10:10AM, Dan Carpenter wrote: > Setting "data = 0" will silence the KMSAN warnings but it doesn't fix > the bug which is that r8712_usbctrl_vendorreq() treats partial reads > as success. > > The usb_control_msg() returns negatives on total failure and it returns > small positives on partial failure. So take the code that I gave you > before and put that into r8712_usbctrl_vendorreq(). That's patch 1. > These patches become 2 and 3. Ah, you mentioned in previous review "But then another problem is that "status" can be less than "len"." I missed it then. - w > > status = usb_control_msg(); > if (status < 0) > goto free; > if (status != len) { > status = -EREMOTEIO; > goto free; > } > if (requesttype == 0x01) > memcpy(pdata, pIo_buf, status); > > regards, > dan carpenter > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends 2022-05-06 3:15 ` [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends Wang Cheng 2022-05-06 7:10 ` Dan Carpenter @ 2022-05-06 7:23 ` Pavel Skripkin 2022-05-06 7:57 ` Dan Carpenter 1 sibling, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-05-06 7:23 UTC (permalink / raw) To: Wang Cheng, dan.carpenter; +Cc: linux-staging, linux-kernel Hi Wang, On 5/6/22 06:15, Wang Cheng wrote: > When r8712_usbctrl_vendorreq() returns negative, 'data' in > usb_read{8,16,32} will not be initialized. [code snip] > + status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, > + &data, len, requesttype); > + if (status < 0) > + return 0; > return le32_to_cpu(data); > } > Why do you believe that 0 is not valid register value? And if it's possible then how you can identify an error? With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends 2022-05-06 7:23 ` Pavel Skripkin @ 2022-05-06 7:57 ` Dan Carpenter 0 siblings, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2022-05-06 7:57 UTC (permalink / raw) To: Pavel Skripkin; +Cc: Wang Cheng, linux-staging, linux-kernel On Fri, May 06, 2022 at 10:23:39AM +0300, Pavel Skripkin wrote: > Hi Wang, > > On 5/6/22 06:15, Wang Cheng wrote: > > When r8712_usbctrl_vendorreq() returns negative, 'data' in > > usb_read{8,16,32} will not be initialized. > > [code snip] > > > + status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, > > + &data, len, requesttype); > > + if (status < 0) > > + return 0; > > return le32_to_cpu(data); > > } > > Why do you believe that 0 is not valid register value? And if it's possible > then how you can identify an error? I think you are getting data and status mixed up? regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 3:13 [PATCH v2 0/2] staging: rtl8712: fix uninit-value 'data' and 'mac' Wang Cheng 2022-05-06 3:15 ` [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends Wang Cheng @ 2022-05-06 3:16 ` Wang Cheng 2022-05-06 7:41 ` Pavel Skripkin 1 sibling, 1 reply; 12+ messages in thread From: Wang Cheng @ 2022-05-06 3:16 UTC (permalink / raw) To: dan.carpenter, paskripkin; +Cc: linux-staging, linux-kernel When 'tmpU1b' returns from r8712_read8(padapter, EE_9346CR) is 0, 'mac[6]' will not be initialized. BUG: KMSAN: uninit-value in r871xu_drv_init+0x2d54/0x3070 drivers/staging/rtl8712/usb_intf.c:541 r871xu_drv_init+0x2d54/0x3070 drivers/staging/rtl8712/usb_intf.c:541 usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396 really_probe+0x653/0x14b0 drivers/base/dd.c:596 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:752 driver_probe_device drivers/base/dd.c:782 [inline] __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:899 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427 __device_attach+0x593/0x8e0 drivers/base/dd.c:970 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1017 bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487 device_add+0x1fff/0x26e0 drivers/base/core.c:3405 usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170 usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238 usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293 really_probe+0x653/0x14b0 drivers/base/dd.c:596 __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:752 driver_probe_device drivers/base/dd.c:782 [inline] __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:899 bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427 __device_attach+0x593/0x8e0 drivers/base/dd.c:970 device_initial_probe+0x4a/0x60 drivers/base/dd.c:1017 bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487 device_add+0x1fff/0x26e0 drivers/base/core.c:3405 usb_new_device+0x1b8e/0x2950 drivers/usb/core/hub.c:2566 hub_port_connect drivers/usb/core/hub.c:5358 [inline] hub_port_connect_change drivers/usb/core/hub.c:5502 [inline] port_event drivers/usb/core/hub.c:5660 [inline] hub_event+0x58e3/0x89e0 drivers/usb/core/hub.c:5742 process_one_work+0xdb6/0x1820 kernel/workqueue.c:2307 worker_thread+0x10b3/0x21e0 kernel/workqueue.c:2454 kthread+0x3c7/0x500 kernel/kthread.c:377 ret_from_fork+0x1f/0x30 Local variable mac created at: r871xu_drv_init+0x1771/0x3070 drivers/staging/rtl8712/usb_intf.c:394 usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396 Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com Signed-off-by: Wang Cheng <wanngchenng@gmail.com> --- drivers/staging/rtl8712/usb_intf.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c index ee4c61f85a07..50dcd3ecb685 100644 --- a/drivers/staging/rtl8712/usb_intf.c +++ b/drivers/staging/rtl8712/usb_intf.c @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, } else { AutoloadFail = false; } - if (((mac[0] == 0xff) && (mac[1] == 0xff) && + if ((!AutoloadFail) || + ((mac[0] == 0xff) && (mac[1] == 0xff) && (mac[2] == 0xff) && (mac[3] == 0xff) && (mac[4] == 0xff) && (mac[5] == 0xff)) || ((mac[0] == 0x00) && (mac[1] == 0x00) && (mac[2] == 0x00) && (mac[3] == 0x00) && - (mac[4] == 0x00) && (mac[5] == 0x00)) || - (!AutoloadFail)) { + (mac[4] == 0x00) && (mac[5] == 0x00))) { mac[0] = 0x00; mac[1] = 0xe0; mac[2] = 0x4c; -- 2.33.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 3:16 ` [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() Wang Cheng @ 2022-05-06 7:41 ` Pavel Skripkin 2022-05-06 11:33 ` Wang Cheng 0 siblings, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-05-06 7:41 UTC (permalink / raw) To: Wang Cheng, dan.carpenter; +Cc: linux-staging, linux-kernel Hi Wang, On 5/6/22 06:16, Wang Cheng wrote: [snip] > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > --- > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > index ee4c61f85a07..50dcd3ecb685 100644 > --- a/drivers/staging/rtl8712/usb_intf.c > +++ b/drivers/staging/rtl8712/usb_intf.c > @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > } else { > AutoloadFail = false; > } > - if (((mac[0] == 0xff) && (mac[1] == 0xff) && > + if ((!AutoloadFail) || > + ((mac[0] == 0xff) && (mac[1] == 0xff) && > (mac[2] == 0xff) && (mac[3] == 0xff) && > (mac[4] == 0xff) && (mac[5] == 0xff)) || > ((mac[0] == 0x00) && (mac[1] == 0x00) && > (mac[2] == 0x00) && (mac[3] == 0x00) && > - (mac[4] == 0x00) && (mac[5] == 0x00)) || > - (!AutoloadFail)) { > + (mac[4] == 0x00) && (mac[5] == 0x00))) { That looks ugly. I mean mac checks. Can we, please, use sane kernel API like is_valid_ether_addr()? > mac[0] = 0x00; > mac[1] = 0xe0; > mac[2] = 0x4c; With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 7:41 ` Pavel Skripkin @ 2022-05-06 11:33 ` Wang Cheng 2022-05-06 11:56 ` Wang Cheng 2022-05-06 12:02 ` Pavel Skripkin 0 siblings, 2 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 11:33 UTC (permalink / raw) To: Pavel Skripkin; +Cc: dan.carpenter, linux-staging, linux-kernel On 22/05/06 10:41AM, Pavel Skripkin wrote: > Hi Wang, > > On 5/6/22 06:16, Wang Cheng wrote: > > [snip] > > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > --- > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > index ee4c61f85a07..50dcd3ecb685 100644 > > --- a/drivers/staging/rtl8712/usb_intf.c > > +++ b/drivers/staging/rtl8712/usb_intf.c > > @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > } else { > > AutoloadFail = false; > > } > > - if (((mac[0] == 0xff) && (mac[1] == 0xff) && > > + if ((!AutoloadFail) || > > + ((mac[0] == 0xff) && (mac[1] == 0xff) && > > (mac[2] == 0xff) && (mac[3] == 0xff) && > > (mac[4] == 0xff) && (mac[5] == 0xff)) || > > ((mac[0] == 0x00) && (mac[1] == 0x00) && > > (mac[2] == 0x00) && (mac[3] == 0x00) && > > - (mac[4] == 0x00) && (mac[5] == 0x00)) || > > - (!AutoloadFail)) { > > + (mac[4] == 0x00) && (mac[5] == 0x00))) { > > > That looks ugly. I mean mac checks. Can we, please, use sane kernel API like > is_valid_ether_addr()? Good idea! But will is_valid_ether_addr() check a larger range? The comment of is_valid_ether_addr()(include/linux/etherdevice.h) says: Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not a *multicast address*, and is not FF:FF:FF:FF:FF:FF. thanks, - w > > > > mac[0] = 0x00; > > mac[1] = 0xe0; > > mac[2] = 0x4c; > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 11:33 ` Wang Cheng @ 2022-05-06 11:56 ` Wang Cheng 2022-05-06 12:02 ` Pavel Skripkin 1 sibling, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 11:56 UTC (permalink / raw) To: Pavel Skripkin; +Cc: dan.carpenter, linux-staging, linux-kernel On 22/05/06 07:33PM, Wang Cheng wrote: > On 22/05/06 10:41AM, Pavel Skripkin wrote: > > Hi Wang, > > > > On 5/6/22 06:16, Wang Cheng wrote: > > > > [snip] > > > > > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > > --- > > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c > > > index ee4c61f85a07..50dcd3ecb685 100644 > > > --- a/drivers/staging/rtl8712/usb_intf.c > > > +++ b/drivers/staging/rtl8712/usb_intf.c > > > @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > > } else { > > > AutoloadFail = false; > > > } > > > - if (((mac[0] == 0xff) && (mac[1] == 0xff) && > > > + if ((!AutoloadFail) || > > > + ((mac[0] == 0xff) && (mac[1] == 0xff) && > > > (mac[2] == 0xff) && (mac[3] == 0xff) && > > > (mac[4] == 0xff) && (mac[5] == 0xff)) || > > > ((mac[0] == 0x00) && (mac[1] == 0x00) && > > > (mac[2] == 0x00) && (mac[3] == 0x00) && > > > - (mac[4] == 0x00) && (mac[5] == 0x00)) || > > > - (!AutoloadFail)) { > > > + (mac[4] == 0x00) && (mac[5] == 0x00))) { > > > > > > That looks ugly. I mean mac checks. Can we, please, use sane kernel API like > > is_valid_ether_addr()? > > Good idea! But will is_valid_ether_addr() check a larger range? > The comment of is_valid_ether_addr()(include/linux/etherdevice.h) says: > Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not > a *multicast address*, and is not FF:FF:FF:FF:FF:FF. is_zero_ether_addr() in etherdevice.h could check the all zero mac address. thanks, - w > > > > > > > > > mac[0] = 0x00; > > > mac[1] = 0xe0; > > > mac[2] = 0x4c; > > > > > > > > > > With regards, > > Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 11:33 ` Wang Cheng 2022-05-06 11:56 ` Wang Cheng @ 2022-05-06 12:02 ` Pavel Skripkin 2022-05-09 4:03 ` Wang Cheng 1 sibling, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-05-06 12:02 UTC (permalink / raw) To: Wang Cheng; +Cc: dan.carpenter, linux-staging, linux-kernel [-- Attachment #1.1: Type: text/plain, Size: 1953 bytes --] Hi Wang, On 5/6/22 14:33, Wang Cheng wrote: > On 22/05/06 10:41AM, Pavel Skripkin wrote: >> Hi Wang, >> >> On 5/6/22 06:16, Wang Cheng wrote: >> >> [snip] >> >> > >> > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com >> > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> >> > --- >> > drivers/staging/rtl8712/usb_intf.c | 6 +++--- >> > 1 file changed, 3 insertions(+), 3 deletions(-) >> > >> > diff --git a/drivers/staging/rtl8712/usb_intf.c b/drivers/staging/rtl8712/usb_intf.c >> > index ee4c61f85a07..50dcd3ecb685 100644 >> > --- a/drivers/staging/rtl8712/usb_intf.c >> > +++ b/drivers/staging/rtl8712/usb_intf.c >> > @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, >> > } else { >> > AutoloadFail = false; >> > } >> > - if (((mac[0] == 0xff) && (mac[1] == 0xff) && >> > + if ((!AutoloadFail) || >> > + ((mac[0] == 0xff) && (mac[1] == 0xff) && >> > (mac[2] == 0xff) && (mac[3] == 0xff) && >> > (mac[4] == 0xff) && (mac[5] == 0xff)) || >> > ((mac[0] == 0x00) && (mac[1] == 0x00) && >> > (mac[2] == 0x00) && (mac[3] == 0x00) && >> > - (mac[4] == 0x00) && (mac[5] == 0x00)) || >> > - (!AutoloadFail)) { >> > + (mac[4] == 0x00) && (mac[5] == 0x00))) { >> >> >> That looks ugly. I mean mac checks. Can we, please, use sane kernel API like >> is_valid_ether_addr()? > > Good idea! But will is_valid_ether_addr() check a larger range? > The comment of is_valid_ether_addr()(include/linux/etherdevice.h) says: > Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not > a *multicast address*, and is not FF:FF:FF:FF:FF:FF. > I am not good an expert at networking stuff, but can multicast mac address be valid for purpose of Wi-Fi adapter? IIUC it's can't, but as I said before, I am not an expert With regards, Pavel Skripkin [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() 2022-05-06 12:02 ` Pavel Skripkin @ 2022-05-09 4:03 ` Wang Cheng 0 siblings, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-09 4:03 UTC (permalink / raw) To: Pavel Skripkin; +Cc: dan.carpenter, linux-staging, linux-kernel On 22/05/06 03:02PM, Pavel Skripkin wrote: > Hi Wang, > > On 5/6/22 14:33, Wang Cheng wrote: > > On 22/05/06 10:41AM, Pavel Skripkin wrote: > > > Hi Wang, > > > > > > On 5/6/22 06:16, Wang Cheng wrote: > > > > > > [snip] > > > > > > > > Reported-and-tested-by: > > > syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > > > --- > > > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > diff --git a/drivers/staging/rtl8712/usb_intf.c > > > b/drivers/staging/rtl8712/usb_intf.c > > > > index ee4c61f85a07..50dcd3ecb685 100644 > > > > --- a/drivers/staging/rtl8712/usb_intf.c > > > > +++ b/drivers/staging/rtl8712/usb_intf.c > > > > @@ -538,13 +538,13 @@ static int r871xu_drv_init(struct usb_interface *pusb_intf, > > > > } else { > > > > AutoloadFail = false; > > > > } > > > > - if (((mac[0] == 0xff) && (mac[1] == 0xff) && > > > > + if ((!AutoloadFail) || > > > > + ((mac[0] == 0xff) && (mac[1] == 0xff) && > > > > (mac[2] == 0xff) && (mac[3] == 0xff) && > > > > (mac[4] == 0xff) && (mac[5] == 0xff)) || > > > > ((mac[0] == 0x00) && (mac[1] == 0x00) && > > > > (mac[2] == 0x00) && (mac[3] == 0x00) && > > > > - (mac[4] == 0x00) && (mac[5] == 0x00)) || > > > > - (!AutoloadFail)) { > > > > + (mac[4] == 0x00) && (mac[5] == 0x00))) { > > > > > > > > > That looks ugly. I mean mac checks. Can we, please, use sane kernel API like > > > is_valid_ether_addr()? > > > > Good idea! But will is_valid_ether_addr() check a larger range? > > The comment of is_valid_ether_addr()(include/linux/etherdevice.h) says: > > Check that the Ethernet address (MAC) is not 00:00:00:00:00:00, is not > > a *multicast address*, and is not FF:FF:FF:FF:FF:FF. > > > > I am not good an expert at networking stuff, but can multicast mac address > be valid for purpose of Wi-Fi adapter? IIUC it's can't, but as I said > before, I am not an expert Me neither. :P I found some reference here: https://en.wikipedia.org/wiki/Multicast_address#Ethernet And in the implementation of is_multicast_ether_addr()(include/linux/etherdevice.h), it checks one bit of mac address to determine a multicast or not. Probably some other kernel API could help. thanks, - w ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-09 4:03 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-05-06 3:13 [PATCH v2 0/2] staging: rtl8712: fix uninit-value 'data' and 'mac' Wang Cheng 2022-05-06 3:15 ` [PATCH v2 1/2] staging: rtl8712: fix uninit-value in usb_read8() and friends Wang Cheng 2022-05-06 7:10 ` Dan Carpenter 2022-05-06 11:22 ` Wang Cheng 2022-05-06 7:23 ` Pavel Skripkin 2022-05-06 7:57 ` Dan Carpenter 2022-05-06 3:16 ` [PATCH v2 2/2] staging: rtl8712: fix uninit-value in r871xu_drv_init() Wang Cheng 2022-05-06 7:41 ` Pavel Skripkin 2022-05-06 11:33 ` Wang Cheng 2022-05-06 11:56 ` Wang Cheng 2022-05-06 12:02 ` Pavel Skripkin 2022-05-09 4:03 ` Wang Cheng
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).