* [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" @ 2022-04-14 14:12 Wang Cheng 2022-04-14 15:42 ` Dan Carpenter 2022-04-14 20:12 ` Pavel Skripkin 0 siblings, 2 replies; 12+ messages in thread From: Wang Cheng @ 2022-04-14 14:12 UTC (permalink / raw) To: Larry.Finger, florian.c.schilhabel, gregkh; +Cc: linux-staging, linux-kernel Due to the case that "requesttype == 0x01 && status <= 0" isn't handled in r8712_usbctrl_vendorreq(), "data" (drivers/staging/rtl8712/usb_ops.c:32) will be returned without initialization. When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) is 0, mac[6] (usb_intf.c:394) won't be initialized, which leads to accessing uninit-value on usb_intf.c:541. Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com Signed-off-by: Wang Cheng <wanngchenng@gmail.com> --- drivers/staging/rtl8712/usb_intf.c | 6 +++--- drivers/staging/rtl8712/usb_ops_linux.c | 14 ++++++++------ 2 files changed, 11 insertions(+), 9 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; diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c index f984a5ab2c6f..e321ca4453ca 100644 --- a/drivers/staging/rtl8712/usb_ops_linux.c +++ b/drivers/staging/rtl8712/usb_ops_linux.c @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, } status = usb_control_msg(udev, pipe, request, reqtype, value, index, pIo_buf, len, 500); - if (status > 0) { /* Success this control transfer. */ - if (requesttype == 0x01) { - /* For Control read transfer, we have to copy the read - * data from pIo_buf to pdata. - */ - memcpy(pdata, pIo_buf, status); + /* For Control read transfer, copy the read data from pIo_buf to pdata + * when control transfer success; otherwise init *pdata with 0. + */ + if (requesttype == 0x01) { + if (status > 0) + memcpy(pdata, pIo_buf, status); + else + *(u32 *)pdata = 0; } } kfree(palloc_buf); -- 2.33.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-14 14:12 [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" Wang Cheng @ 2022-04-14 15:42 ` Dan Carpenter 2022-04-14 15:49 ` Dan Carpenter 2022-04-15 9:47 ` Wang Cheng 2022-04-14 20:12 ` Pavel Skripkin 1 sibling, 2 replies; 12+ messages in thread From: Dan Carpenter @ 2022-04-14 15:42 UTC (permalink / raw) To: Wang Cheng Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On Thu, Apr 14, 2022 at 10:12:23PM +0800, Wang Cheng wrote: > Due to the case that "requesttype == 0x01 && status <= 0" > isn't handled in r8712_usbctrl_vendorreq(), > "data" (drivers/staging/rtl8712/usb_ops.c:32) > will be returned without initialization. > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > is 0, mac[6] (usb_intf.c:394) won't be initialized, > which leads to accessing uninit-value on usb_intf.c:541. These line numbers are sort of useless because everyone is on a different git hash. > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > --- > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > drivers/staging/rtl8712/usb_ops_linux.c | 14 ++++++++------ > 2 files changed, 11 insertions(+), 9 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; This is a separate fix from the rest of the patch. Send it by itself. > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > index f984a5ab2c6f..e321ca4453ca 100644 > --- a/drivers/staging/rtl8712/usb_ops_linux.c > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > } > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > pIo_buf, len, 500); > - if (status > 0) { /* Success this control transfer. */ > - if (requesttype == 0x01) { > - /* For Control read transfer, we have to copy the read > - * data from pIo_buf to pdata. > - */ > - memcpy(pdata, pIo_buf, status); > + /* For Control read transfer, copy the read data from pIo_buf to pdata > + * when control transfer success; otherwise init *pdata with 0. > + */ > + if (requesttype == 0x01) { > + if (status > 0) > + memcpy(pdata, pIo_buf, status); > + else > + *(u32 *)pdata = 0; > } This isn't really correct. In many cases status is "len" is less than 4. I'm slightly surprised that nothing complains about that as an uninitialized access. But then another problem is that "status" can be less than "len". A better fix instead of setting pdata to zero would be to add error checking in the callers and then change this code to use usb_control_msg_send/recv(). Probably just initialize "data" in the callers as well. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-14 15:42 ` Dan Carpenter @ 2022-04-14 15:49 ` Dan Carpenter 2022-04-15 9:47 ` Wang Cheng 1 sibling, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2022-04-14 15:49 UTC (permalink / raw) To: Wang Cheng Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On Thu, Apr 14, 2022 at 06:42:15PM +0300, Dan Carpenter wrote: > > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > > index f984a5ab2c6f..e321ca4453ca 100644 > > --- a/drivers/staging/rtl8712/usb_ops_linux.c > > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > > } > > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > > pIo_buf, len, 500); > > - if (status > 0) { /* Success this control transfer. */ > > - if (requesttype == 0x01) { > > - /* For Control read transfer, we have to copy the read > > - * data from pIo_buf to pdata. > > - */ > > - memcpy(pdata, pIo_buf, status); > > + /* For Control read transfer, copy the read data from pIo_buf to pdata > > + * when control transfer success; otherwise init *pdata with 0. > > + */ > > + if (requesttype == 0x01) { > > + if (status > 0) > > + memcpy(pdata, pIo_buf, status); > > + else > > + *(u32 *)pdata = 0; > > } > > This isn't really correct. In many cases status is "len" is less than 4. > I'm slightly surprised that nothing complains about that as an > uninitialized access. But then another problem is that "status" can be > less than "len". > > A better fix instead of setting pdata to zero would be to add error > checking in the callers and then change this code to use > usb_control_msg_send/recv(). Probably just initialize "data" in the > callers as well. A different, less good option would be to, still add error handling in the callers, but do: status = usb_control_msg(); if (status < 0) goto free; if (status != len) { status = -EREMOTEIO; goto free; } if (requesttype == 0x01) memcpy(pdata, pIo_buf, status); status = 0; free: kfree(palloc_buf); return status; regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-14 15:42 ` Dan Carpenter 2022-04-14 15:49 ` Dan Carpenter @ 2022-04-15 9:47 ` Wang Cheng 2022-04-15 9:57 ` Dan Carpenter 2022-04-15 10:02 ` Dan Carpenter 1 sibling, 2 replies; 12+ messages in thread From: Wang Cheng @ 2022-04-15 9:47 UTC (permalink / raw) To: Dan Carpenter Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel Hi Dan, thx for your review. On 22/04/14 06:42PM, Dan Carpenter wrote: > On Thu, Apr 14, 2022 at 10:12:23PM +0800, Wang Cheng wrote: > > Due to the case that "requesttype == 0x01 && status <= 0" > > isn't handled in r8712_usbctrl_vendorreq(), > > "data" (drivers/staging/rtl8712/usb_ops.c:32) > > will be returned without initialization. > > > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > > is 0, mac[6] (usb_intf.c:394) won't be initialized, > > which leads to accessing uninit-value on usb_intf.c:541. > > These line numbers are sort of useless because everyone is on a > different git hash. I will correct this. > > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > --- > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > drivers/staging/rtl8712/usb_ops_linux.c | 14 ++++++++------ > > 2 files changed, 11 insertions(+), 9 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; > > This is a separate fix from the rest of the patch. Send it by itself. Ah, thought to send a patch series. > > > > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > > index f984a5ab2c6f..e321ca4453ca 100644 > > --- a/drivers/staging/rtl8712/usb_ops_linux.c > > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > > } > > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > > pIo_buf, len, 500); > > - if (status > 0) { /* Success this control transfer. */ > > - if (requesttype == 0x01) { > > - /* For Control read transfer, we have to copy the read > > - * data from pIo_buf to pdata. > > - */ > > - memcpy(pdata, pIo_buf, status); > > + /* For Control read transfer, copy the read data from pIo_buf to pdata > > + * when control transfer success; otherwise init *pdata with 0. > > + */ > > + if (requesttype == 0x01) { > > + if (status > 0) > > + memcpy(pdata, pIo_buf, status); > > + else > > + *(u32 *)pdata = 0; > > } > > This isn't really correct. In many cases status is "len" is less than 4. > I'm slightly surprised that nothing complains about that as an > uninitialized access. But then another problem is that "status" can be > less than "len". Sorry, I should explain it clearly. If I did right, watching "status" with gdb while running syzkaller reproducer, "status" returns from usb_control_msg() is -71. In which case, *pdata won't be touched in r8712_usbctrl_vendorreq(). As a result, "data" in usb_read8()/usb_read16()/usb_read32() will be returned without initialization. I think that is why kmsan reports: 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 > > A better fix instead of setting pdata to zero would be to add error > checking in the callers and then change this code to use > usb_control_msg_send/recv(). Probably just initialize "data" in the > callers as well. I tried something similar which also works fine, but I think this patch does't fix it at root. https://syzkaller.appspot.com/text?tag=Patch&x=15be2970f00000 > > regards, > dan carpenter > thanks, -- w ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-15 9:47 ` Wang Cheng @ 2022-04-15 9:57 ` Dan Carpenter 2022-04-15 10:51 ` Wang Cheng 2022-05-06 3:07 ` Wang Cheng 2022-04-15 10:02 ` Dan Carpenter 1 sibling, 2 replies; 12+ messages in thread From: Dan Carpenter @ 2022-04-15 9:57 UTC (permalink / raw) To: Wang Cheng Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On Fri, Apr 15, 2022 at 05:47:05PM +0800, Wang Cheng wrote: > Hi Dan, thx for your review. > > On 22/04/14 06:42PM, Dan Carpenter wrote: > > On Thu, Apr 14, 2022 at 10:12:23PM +0800, Wang Cheng wrote: > > > Due to the case that "requesttype == 0x01 && status <= 0" > > > isn't handled in r8712_usbctrl_vendorreq(), > > > "data" (drivers/staging/rtl8712/usb_ops.c:32) > > > will be returned without initialization. > > > > > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > > > is 0, mac[6] (usb_intf.c:394) won't be initialized, > > > which leads to accessing uninit-value on usb_intf.c:541. > > > > These line numbers are sort of useless because everyone is on a > > different git hash. > > I will correct this. > > > > > > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > > --- > > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > > drivers/staging/rtl8712/usb_ops_linux.c | 14 ++++++++------ > > > 2 files changed, 11 insertions(+), 9 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; > > > > This is a separate fix from the rest of the patch. Send it by itself. > > Ah, thought to send a patch series. > Yes, please. Send two patches. > > > > > > > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > > > index f984a5ab2c6f..e321ca4453ca 100644 > > > --- a/drivers/staging/rtl8712/usb_ops_linux.c > > > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > > > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > > > } > > > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > > > pIo_buf, len, 500); > > > - if (status > 0) { /* Success this control transfer. */ > > > - if (requesttype == 0x01) { > > > - /* For Control read transfer, we have to copy the read > > > - * data from pIo_buf to pdata. > > > - */ > > > - memcpy(pdata, pIo_buf, status); > > > + /* For Control read transfer, copy the read data from pIo_buf to pdata > > > + * when control transfer success; otherwise init *pdata with 0. > > > + */ > > > + if (requesttype == 0x01) { > > > + if (status > 0) > > > + memcpy(pdata, pIo_buf, status); > > > + else > > > + *(u32 *)pdata = 0; > > > } > > > > This isn't really correct. In many cases status is "len" is less than 4. > > I'm slightly surprised that nothing complains about that as an > > uninitialized access. But then another problem is that "status" can be > > less than "len". > > Sorry, I should explain it clearly. If I did right, watching "status" > with gdb while running syzkaller reproducer, "status" returns from > usb_control_msg() is -71. In which case, *pdata won't be touched in > r8712_usbctrl_vendorreq(). As a result, "data" in > usb_read8()/usb_read16()/usb_read32() will be returned without > initialization. I think that is why kmsan reports: > 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 Yes. I understood that. > > > > > A better fix instead of setting pdata to zero would be to add error > > checking in the callers and then change this code to use > > usb_control_msg_send/recv(). Probably just initialize "data" in the > > callers as well. > > I tried something similar which also works fine, but I think this patch > does't fix it at root. > https://syzkaller.appspot.com/text?tag=Patch&x=15be2970f00000 > Ugh... Sorry, I had not looked closely at usb_read8() and friends. (This code is all so terrible). Ideally they would have some way to return errors. Oh well. Anyway, I guess do like this: status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, &data, len, requesttype); if (status < 0) return 0; return (u8)(le32_to_cpu(data) & 0x0ff); But in r8712_usbctrl_vendorreq() you really need to make the other changes I mentioned as well. If you want to do it as a separate patch that's fine too. if (status < 0) return status; if (status != len) return -EREMOTEIO; if (reqtype == 0x1) memcpy(); return 0; regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-15 9:57 ` Dan Carpenter @ 2022-04-15 10:51 ` Wang Cheng 2022-05-06 3:07 ` Wang Cheng 1 sibling, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-04-15 10:51 UTC (permalink / raw) To: Dan Carpenter Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On 22/04/15 12:57PM, Dan Carpenter wrote: > On Fri, Apr 15, 2022 at 05:47:05PM +0800, Wang Cheng wrote: > > Hi Dan, thx for your review. > > > > On 22/04/14 06:42PM, Dan Carpenter wrote: > > > On Thu, Apr 14, 2022 at 10:12:23PM +0800, Wang Cheng wrote: > > > > Due to the case that "requesttype == 0x01 && status <= 0" > > > > isn't handled in r8712_usbctrl_vendorreq(), > > > > "data" (drivers/staging/rtl8712/usb_ops.c:32) > > > > will be returned without initialization. > > > > > > > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > > > > is 0, mac[6] (usb_intf.c:394) won't be initialized, > > > > which leads to accessing uninit-value on usb_intf.c:541. > > > > > > These line numbers are sort of useless because everyone is on a > > > different git hash. > > > > I will correct this. > > > > > > > > > > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > > > --- > > > > drivers/staging/rtl8712/usb_intf.c | 6 +++--- > > > > drivers/staging/rtl8712/usb_ops_linux.c | 14 ++++++++------ > > > > 2 files changed, 11 insertions(+), 9 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; > > > > > > This is a separate fix from the rest of the patch. Send it by itself. > > > > Ah, thought to send a patch series. > > > > Yes, please. Send two patches. > > > > > > > > > > > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > > > > index f984a5ab2c6f..e321ca4453ca 100644 > > > > --- a/drivers/staging/rtl8712/usb_ops_linux.c > > > > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > > > > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > > > > } > > > > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > > > > pIo_buf, len, 500); > > > > - if (status > 0) { /* Success this control transfer. */ > > > > - if (requesttype == 0x01) { > > > > - /* For Control read transfer, we have to copy the read > > > > - * data from pIo_buf to pdata. > > > > - */ > > > > - memcpy(pdata, pIo_buf, status); > > > > + /* For Control read transfer, copy the read data from pIo_buf to pdata > > > > + * when control transfer success; otherwise init *pdata with 0. > > > > + */ > > > > + if (requesttype == 0x01) { > > > > + if (status > 0) > > > > + memcpy(pdata, pIo_buf, status); > > > > + else > > > > + *(u32 *)pdata = 0; > > > > } > > > > > > This isn't really correct. In many cases status is "len" is less than 4. > > > I'm slightly surprised that nothing complains about that as an > > > uninitialized access. But then another problem is that "status" can be > > > less than "len". > > > > Sorry, I should explain it clearly. If I did right, watching "status" > > with gdb while running syzkaller reproducer, "status" returns from > > usb_control_msg() is -71. In which case, *pdata won't be touched in > > r8712_usbctrl_vendorreq(). As a result, "data" in > > usb_read8()/usb_read16()/usb_read32() will be returned without > > initialization. I think that is why kmsan reports: > > 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 > > Yes. I understood that. > > > > > > > > > A better fix instead of setting pdata to zero would be to add error > > > checking in the callers and then change this code to use > > > usb_control_msg_send/recv(). Probably just initialize "data" in the > > > callers as well. > > > > I tried something similar which also works fine, but I think this patch > > does't fix it at root. > > https://syzkaller.appspot.com/text?tag=Patch&x=15be2970f00000 > > > > Ugh... Sorry, I had not looked closely at usb_read8() and friends. > (This code is all so terrible). Ideally they would have some way to > return errors. Oh well. Anyway, I guess do like this: > > status = r8712_usbctrl_vendorreq(intfpriv, request, wvalue, index, > &data, len, requesttype); > if (status < 0) > return 0; > return (u8)(le32_to_cpu(data) & 0x0ff); > > But in r8712_usbctrl_vendorreq() you really need to make the other > changes I mentioned as well. If you want to do it as a separate patch > that's fine too. > > if (status < 0) > return status; > if (status != len) > return -EREMOTEIO; > > if (reqtype == 0x1) > memcpy(); > > return 0; Thx for your time and advice, Dan. Yeah, the code logic looks not easy. I will think about these code snippets and handle them. thanks, -- w ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-15 9:57 ` Dan Carpenter 2022-04-15 10:51 ` Wang Cheng @ 2022-05-06 3:07 ` Wang Cheng 1 sibling, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-05-06 3:07 UTC (permalink / raw) To: dan.carpenter, paskripkin; +Cc: linux-staging, linux-kernel On 22/04/15 12:57PM, Dan Carpenter wrote: > > But in r8712_usbctrl_vendorreq() you really need to make the other > changes I mentioned as well. If you want to do it as a separate patch > that's fine too. > > if (status < 0) > return status; > if (status != len) > return -EREMOTEIO; > > if (reqtype == 0x1) > memcpy(); > > return 0; Hi Dan and Pavel, Sorry for the late reply. A v2 patch series is prepared, in which the uninit-value bug is fixed by adding error checking and initialization. In that case, r8712_usbctrl_vendorreq() is not touched any more. So WDYT to refactor r8712_usbctrl_vendorreq() in another patch with the code Dan mentioned above and usb_control_msg_{recv,send} suggested by Pavel? And for now, the patch series is just confined to fixing uninit-values. The patches will each focus on one thing that is easy to understand. thanks, - w ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-15 9:47 ` Wang Cheng 2022-04-15 9:57 ` Dan Carpenter @ 2022-04-15 10:02 ` Dan Carpenter 1 sibling, 0 replies; 12+ messages in thread From: Dan Carpenter @ 2022-04-15 10:02 UTC (permalink / raw) To: Wang Cheng Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On Fri, Apr 15, 2022 at 05:47:05PM +0800, Wang Cheng wrote: > > > diff --git a/drivers/staging/rtl8712/usb_ops_linux.c b/drivers/staging/rtl8712/usb_ops_linux.c > > > index f984a5ab2c6f..e321ca4453ca 100644 > > > --- a/drivers/staging/rtl8712/usb_ops_linux.c > > > +++ b/drivers/staging/rtl8712/usb_ops_linux.c > > > @@ -495,12 +495,14 @@ int r8712_usbctrl_vendorreq(struct intf_priv *pintfpriv, u8 request, u16 value, > > > } > > > status = usb_control_msg(udev, pipe, request, reqtype, value, index, > > > pIo_buf, len, 500); > > > - if (status > 0) { /* Success this control transfer. */ > > > - if (requesttype == 0x01) { > > > - /* For Control read transfer, we have to copy the read > > > - * data from pIo_buf to pdata. > > > - */ > > > - memcpy(pdata, pIo_buf, status); > > > + /* For Control read transfer, copy the read data from pIo_buf to pdata > > > + * when control transfer success; otherwise init *pdata with 0. > > > + */ > > > + if (requesttype == 0x01) { > > > + if (status > 0) > > > + memcpy(pdata, pIo_buf, status); > > > + else > > > + *(u32 *)pdata = 0; > > > } > > > > This isn't really correct. In many cases status is "len" is less than 4. > > I'm slightly surprised that nothing complains about that as an > > uninitialized access. But then another problem is that "status" can be > > less than "len". > > Sorry, I should explain it clearly. If I did right, watching "status" > with gdb while running syzkaller reproducer, "status" returns from > usb_control_msg() is -71. In which case, *pdata won't be touched in > r8712_usbctrl_vendorreq(). As a result, "data" in > usb_read8()/usb_read16()/usb_read32() will be returned without > initialization. I think that is why kmsan reports: > 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 > > > > > A better fix instead of setting pdata to zero would be to add error > > checking in the callers and then change this code to use > > usb_control_msg_send/recv(). Probably just initialize "data" in the > > callers as well. > > I tried something similar which also works fine, but I think this patch > does't fix it at root. > https://syzkaller.appspot.com/text?tag=Patch&x=15be2970f00000 Oh... I meant initialize data on the success path as well. __le32 data = 0; You're saying that kasan generates a warning for your updated patch? Earlier I mentioned "I'm slightly surprised that nothing complains about that as an uninitialized access." so I guess maybe it does complain? Good. regards, dan carpenter ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-14 14:12 [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" Wang Cheng 2022-04-14 15:42 ` Dan Carpenter @ 2022-04-14 20:12 ` Pavel Skripkin 2022-04-15 9:57 ` Wang Cheng 1 sibling, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-04-14 20:12 UTC (permalink / raw) To: Wang Cheng, Larry.Finger, florian.c.schilhabel, gregkh Cc: linux-staging, linux-kernel Hi Wang, On 4/14/22 17:12, Wang Cheng wrote: > Due to the case that "requesttype == 0x01 && status <= 0" > isn't handled in r8712_usbctrl_vendorreq(), > "data" (drivers/staging/rtl8712/usb_ops.c:32) > will be returned without initialization. > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > is 0, mac[6] (usb_intf.c:394) won't be initialized, > which leads to accessing uninit-value on usb_intf.c:541. > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> This patch will just hide the problematic API in that driver. Correct fix is changing usb_control_msg to usb_control_msg_{recv,send}. IIRC this driver does not want read various length requests, so it should be fine With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-14 20:12 ` Pavel Skripkin @ 2022-04-15 9:57 ` Wang Cheng 2022-04-16 10:57 ` Pavel Skripkin 0 siblings, 1 reply; 12+ messages in thread From: Wang Cheng @ 2022-04-15 9:57 UTC (permalink / raw) To: Pavel Skripkin Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On 22/04/14 11:12PM, Pavel Skripkin wrote: > Hi Wang, > > On 4/14/22 17:12, Wang Cheng wrote: > > Due to the case that "requesttype == 0x01 && status <= 0" > > isn't handled in r8712_usbctrl_vendorreq(), > > "data" (drivers/staging/rtl8712/usb_ops.c:32) > > will be returned without initialization. > > > > When "tmpU1b" (drivers/staging/rtl8712/usb_intf.c:395) > > is 0, mac[6] (usb_intf.c:394) won't be initialized, > > which leads to accessing uninit-value on usb_intf.c:541. > > > > Reported-and-tested-by: syzbot+6f5ecd144854c0d8580b@syzkaller.appspotmail.com > > Signed-off-by: Wang Cheng <wanngchenng@gmail.com> > > This patch will just hide the problematic API in that driver. Correct fix is > changing usb_control_msg to usb_control_msg_{recv,send}. > > IIRC this driver does not want read various length requests, so it should be > fine Hi Pavel, thx for your review. Sorry, this patch is just confined to fixing uninit-values with modifying the original code as less as possible. It sounds good to refactor r8712_usbctrl_vendorreq() with better API. thanks, -- w > > > > > With regards, > Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-15 9:57 ` Wang Cheng @ 2022-04-16 10:57 ` Pavel Skripkin 2022-04-18 4:00 ` Wang Cheng 0 siblings, 1 reply; 12+ messages in thread From: Pavel Skripkin @ 2022-04-16 10:57 UTC (permalink / raw) To: Wang Cheng Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel Hi Wang, On 4/15/22 12:57, Wang Cheng wrote: > Hi Pavel, thx for your review. > > Sorry, this patch is just confined to fixing uninit-values with > modifying the original code as less as possible. It sounds good to > refactor r8712_usbctrl_vendorreq() with better API. > I understand your idea, but this will just hide the real problem until syzbot hit it in different place. So, I believe, it's better to fix the root case instead of hiding local cases. With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" 2022-04-16 10:57 ` Pavel Skripkin @ 2022-04-18 4:00 ` Wang Cheng 0 siblings, 0 replies; 12+ messages in thread From: Wang Cheng @ 2022-04-18 4:00 UTC (permalink / raw) To: Pavel Skripkin Cc: Larry.Finger, florian.c.schilhabel, gregkh, linux-staging, linux-kernel On 22/04/16 01:57PM, Pavel Skripkin wrote: > Hi Wang, > > On 4/15/22 12:57, Wang Cheng wrote: > > Hi Pavel, thx for your review. > > > > Sorry, this patch is just confined to fixing uninit-values with > > modifying the original code as less as possible. It sounds good to > > refactor r8712_usbctrl_vendorreq() with better API. > > > > I understand your idea, but this will just hide the real problem until > syzbot hit it in different place. So, I believe, it's better to fix the root > case instead of hiding local cases. Of course, I agree on fixing the root case. I will think about replacing APIs properly. thanks, - w ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-05-06 3:07 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-04-14 14:12 [PATCH] staging: rtl8712: fix uninit-value "data" and "mac" Wang Cheng 2022-04-14 15:42 ` Dan Carpenter 2022-04-14 15:49 ` Dan Carpenter 2022-04-15 9:47 ` Wang Cheng 2022-04-15 9:57 ` Dan Carpenter 2022-04-15 10:51 ` Wang Cheng 2022-05-06 3:07 ` Wang Cheng 2022-04-15 10:02 ` Dan Carpenter 2022-04-14 20:12 ` Pavel Skripkin 2022-04-15 9:57 ` Wang Cheng 2022-04-16 10:57 ` Pavel Skripkin 2022-04-18 4:00 ` 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).