| Message ID | 20220102171943.28846-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [RFT] ieee802154: atusb: move to new USB API | expand |
Hi, On Sun, 2 Jan 2022 at 12:19, Pavel Skripkin <paskripkin@gmail.com> wrote: > > Alexander reported a use of uninitialized value in > atusb_set_extended_addr(), that is caused by reading 0 bytes via > usb_control_msg(). > Does there exist no way to check on this and return an error on USB API caller level? > Since there is an API, that cannot read less bytes, than was requested, > let's move atusb driver to use it. It will fix all potintial bugs with > uninit values and make code more modern > If this is not possible to fix with the "old" USB API then I think the "old" USB API needs to be fixed. Changing to the new USB API as "making the code more modern" is a new feature and is a candidate for next. - Alex
On 1/3/22 01:15, Alexander Aring wrote: > Hi, > > On Sun, 2 Jan 2022 at 12:19, Pavel Skripkin <paskripkin@gmail.com> wrote: >> >> Alexander reported a use of uninitialized value in >> atusb_set_extended_addr(), that is caused by reading 0 bytes via >> usb_control_msg(). >> > > Does there exist no way to check on this and return an error on USB > API caller level? > >> Since there is an API, that cannot read less bytes, than was requested, >> let's move atusb driver to use it. It will fix all potintial bugs with >> uninit values and make code more modern >> > > If this is not possible to fix with the "old" USB API then I think the > "old" USB API needs to be fixed. > Changing to the new USB API as "making the code more modern" is a new > feature and is a candidate for next. > It can be fixed with the old one. Something like that should work: - if (ret < 0) { - atusb->err = ret; + if (ret < size) { + atusb->err = ret < 0: ret: -ENODATA; But I thought, that moving to new API is better fix, just because old one prone to uninit value bugs if error checking is wrong With regards, Pavel Skripkin
Hi, On Sun, 2 Jan 2022 at 17:21, Pavel Skripkin <paskripkin@gmail.com> wrote: > > On 1/3/22 01:15, Alexander Aring wrote: > > Hi, > > > > On Sun, 2 Jan 2022 at 12:19, Pavel Skripkin <paskripkin@gmail.com> wrote: > >> > >> Alexander reported a use of uninitialized value in > >> atusb_set_extended_addr(), that is caused by reading 0 bytes via > >> usb_control_msg(). > >> > > > > Does there exist no way to check on this and return an error on USB > > API caller level? > > > >> Since there is an API, that cannot read less bytes, than was requested, > >> let's move atusb driver to use it. It will fix all potintial bugs with > >> uninit values and make code more modern > >> > > > > If this is not possible to fix with the "old" USB API then I think the > > "old" USB API needs to be fixed. > > Changing to the new USB API as "making the code more modern" is a new > > feature and is a candidate for next. > > > > It can be fixed with the old one. Something like that should work: > > - if (ret < 0) { > - atusb->err = ret; > + if (ret < size) { > + atusb->err = ret < 0: ret: -ENODATA; > > But I thought, that moving to new API is better fix, just because old > one prone to uninit value bugs if error checking is wrong A fix should have the smallest changes as possible and not use "new stuff" which might break other things. Also I am not sure since "when" this new USB API exists. To backport the fix into stable send a fix using the "old USB API". If the fix is upstream you can send patches to use the new API and remove the additional check if this is done by using the new API. Maybe it's worth checking that the errno stays the same. Thanks. - Alex
On Sun, Jan 02, 2022 at 08:19:43PM +0300, Pavel Skripkin wrote: > Alexander reported a use of uninitialized value in > atusb_set_extended_addr(), that is caused by reading 0 bytes via > usb_control_msg(). > > Since there is an API, that cannot read less bytes, than was requested, > let's move atusb driver to use it. It will fix all potintial bugs with > uninit values and make code more modern > > Fail log: > > BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd > ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 > > Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") > Cc: stable@vger.kernel.org # 5.9 > Reported-by: Alexander Potapenko <glider@google.com> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- > 1 file changed, 38 insertions(+), 23 deletions(-) > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > index 23ee0b14cbfa..43befea0110f 100644 > --- a/drivers/net/ieee802154/atusb.c > +++ b/drivers/net/ieee802154/atusb.c > @@ -80,10 +80,9 @@ struct atusb_chip_data { > * in atusb->err and reject all subsequent requests until the error is cleared. > */ > > -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, > - __u8 request, __u8 requesttype, > - __u16 value, __u16 index, > - void *data, __u16 size, int timeout) > +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, > + __u16 value, __u16 index, > + void *data, __u16 size, int timeout) Why do you need a wrapper function at all? Why not just call the real usb functions instead? > { > struct usb_device *usb_dev = atusb->usb_dev; > int ret; > @@ -91,8 +90,30 @@ static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, > if (atusb->err) > return atusb->err; > > - ret = usb_control_msg(usb_dev, pipe, request, requesttype, > - value, index, data, size, timeout); > + ret = usb_control_msg_recv(usb_dev, 0, request, requesttype, > + value, index, data, size, timeout, GFP_KERNEL); > + if (ret < 0) { > + atusb->err = ret; > + dev_err(&usb_dev->dev, > + "%s: req 0x%02x val 0x%x idx 0x%x, error %d\n", > + __func__, request, value, index, ret); > + } Why save off the error value at all? And was that message needed? > + > + return ret; > +} > + > +static int atusb_control_msg_send(struct atusb *atusb, __u8 request, __u8 requesttype, > + __u16 value, __u16 index, > + void *data, __u16 size, int timeout) > +{ > + struct usb_device *usb_dev = atusb->usb_dev; > + int ret; > + > + if (atusb->err) > + return atusb->err; > + > + ret = usb_control_msg_send(usb_dev, 0, request, requesttype, > + value, index, data, size, timeout, GFP_KERNEL); > if (ret < 0) { > atusb->err = ret; > dev_err(&usb_dev->dev, > @@ -107,8 +128,7 @@ static int atusb_command(struct atusb *atusb, u8 cmd, u8 arg) > struct usb_device *usb_dev = atusb->usb_dev; > > dev_dbg(&usb_dev->dev, "%s: cmd = 0x%x\n", __func__, cmd); > - return atusb_control_msg(atusb, usb_sndctrlpipe(usb_dev, 0), > - cmd, ATUSB_REQ_TO_DEV, arg, 0, NULL, 0, 1000); > + return atusb_control_msg_send(atusb, cmd, ATUSB_REQ_TO_DEV, arg, 0, NULL, 0, 1000); > } > > static int atusb_write_reg(struct atusb *atusb, u8 reg, u8 value) > @@ -116,9 +136,8 @@ static int atusb_write_reg(struct atusb *atusb, u8 reg, u8 value) > struct usb_device *usb_dev = atusb->usb_dev; > > dev_dbg(&usb_dev->dev, "%s: 0x%02x <- 0x%02x\n", __func__, reg, value); > - return atusb_control_msg(atusb, usb_sndctrlpipe(usb_dev, 0), > - ATUSB_REG_WRITE, ATUSB_REQ_TO_DEV, > - value, reg, NULL, 0, 1000); > + return atusb_control_msg_send(atusb, ATUSB_REG_WRITE, ATUSB_REQ_TO_DEV, > + value, reg, NULL, 0, 1000); This return value can be different, are you sure you want to call this this way? I would recommend just moving to use the real USB functions and no wrapper function at all like this, it will make things more obvious and easier to understand over time. thanks, greg k-h
On Sun, Jan 02, 2022 at 05:15:38PM -0500, Alexander Aring wrote: > Hi, > > On Sun, 2 Jan 2022 at 12:19, Pavel Skripkin <paskripkin@gmail.com> wrote: > > > > Alexander reported a use of uninitialized value in > > atusb_set_extended_addr(), that is caused by reading 0 bytes via > > usb_control_msg(). > > > > Does there exist no way to check on this and return an error on USB > API caller level? > > > Since there is an API, that cannot read less bytes, than was requested, > > let's move atusb driver to use it. It will fix all potintial bugs with > > uninit values and make code more modern > > > > If this is not possible to fix with the "old" USB API then I think the > "old" USB API needs to be fixed. We can not get rid of the "old" api calls, as sometimes they are needed for some corner cases where you want to know if you read/wrote a shorter/larger message than expected. > Changing to the new USB API as "making the code more modern" is a new > feature and is a candidate for next. Fixing bugs is a good thing to do no matter when it happens. thanks, greg k-h
Hi, On Mon, 3 Jan 2022 at 08:03, Greg KH <greg@kroah.com> wrote: > > On Sun, Jan 02, 2022 at 08:19:43PM +0300, Pavel Skripkin wrote: > > Alexander reported a use of uninitialized value in > > atusb_set_extended_addr(), that is caused by reading 0 bytes via > > usb_control_msg(). > > > > Since there is an API, that cannot read less bytes, than was requested, > > let's move atusb driver to use it. It will fix all potintial bugs with > > uninit values and make code more modern > > > > Fail log: > > > > BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > > BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > > BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > > Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd > > ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > > atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > > atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > > usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 > > > > Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") > > Cc: stable@vger.kernel.org # 5.9 > > Reported-by: Alexander Potapenko <glider@google.com> > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > --- > > drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- > > 1 file changed, 38 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > index 23ee0b14cbfa..43befea0110f 100644 > > --- a/drivers/net/ieee802154/atusb.c > > +++ b/drivers/net/ieee802154/atusb.c > > @@ -80,10 +80,9 @@ struct atusb_chip_data { > > * in atusb->err and reject all subsequent requests until the error is cleared. > > */ > > > > -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, > > - __u8 request, __u8 requesttype, > > - __u16 value, __u16 index, > > - void *data, __u16 size, int timeout) > > +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, > > + __u16 value, __u16 index, > > + void *data, __u16 size, int timeout) > > Why do you need a wrapper function at all? Why not just call the real > usb functions instead? > This driver has a lot of history, there is a comment which states: "To reduce the number of error checks in the code, we record the first error in atusb->err and reject all subsequent requests until the error is cleared." I think in the early state of this driver (as it was acting more as an USB<->SPI bridge) there was a lot of state handling involved. Nowadays we have a lot of such handling inside the device firmware (which is btw. open source). This might be not an excuse but an explanation why it was introduced in such a way. ... > > I would recommend just moving to use the real USB functions and no > wrapper function at all like this, it will make things more obvious and > easier to understand over time. okay. - Alex
Hello. On 03.01.22 16:35, Alexander Aring wrote: > Hi, > > On Mon, 3 Jan 2022 at 08:03, Greg KH <greg@kroah.com> wrote: >> >> On Sun, Jan 02, 2022 at 08:19:43PM +0300, Pavel Skripkin wrote: >>> Alexander reported a use of uninitialized value in >>> atusb_set_extended_addr(), that is caused by reading 0 bytes via >>> usb_control_msg(). >>> >>> Since there is an API, that cannot read less bytes, than was requested, >>> let's move atusb driver to use it. It will fix all potintial bugs with >>> uninit values and make code more modern >>> >>> Fail log: >>> >>> BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] >>> BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] >>> BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 >>> Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd >>> ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] >>> atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] >>> atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 >>> usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 >>> >>> Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") >>> Cc: stable@vger.kernel.org # 5.9 >>> Reported-by: Alexander Potapenko <glider@google.com> >>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >>> --- >>> drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- >>> 1 file changed, 38 insertions(+), 23 deletions(-) >>> >>> diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c >>> index 23ee0b14cbfa..43befea0110f 100644 >>> --- a/drivers/net/ieee802154/atusb.c >>> +++ b/drivers/net/ieee802154/atusb.c >>> @@ -80,10 +80,9 @@ struct atusb_chip_data { >>> * in atusb->err and reject all subsequent requests until the error is cleared. >>> */ >>> >>> -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, >>> - __u8 request, __u8 requesttype, >>> - __u16 value, __u16 index, >>> - void *data, __u16 size, int timeout) >>> +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, >>> + __u16 value, __u16 index, >>> + void *data, __u16 size, int timeout) >> >> Why do you need a wrapper function at all? Why not just call the real >> usb functions instead? > ... >> >> I would recommend just moving to use the real USB functions and no >> wrapper function at all like this, it will make things more obvious and >> easier to understand over time. > > okay. With the small fix handle the actual KASAN report applied now I am happy to work with Pavel to get a patch using the newer USB API tested and applied for -next. Pavel would you be willing to update your patch with the complete removal of the atusb usb wrapper functions? Like Greg suggested. That plus the porting to the newer USB API should be a good step forward. Happy to review and test your patches. regards Stefan Schmidt
On Tue, Jan 04, 2022 at 08:41:23PM +0100, Stefan Schmidt wrote: > Hello. > > On 03.01.22 16:35, Alexander Aring wrote: > > Hi, > > > > On Mon, 3 Jan 2022 at 08:03, Greg KH <greg@kroah.com> wrote: > > > > > > On Sun, Jan 02, 2022 at 08:19:43PM +0300, Pavel Skripkin wrote: > > > > Alexander reported a use of uninitialized value in > > > > atusb_set_extended_addr(), that is caused by reading 0 bytes via > > > > usb_control_msg(). > > > > > > > > Since there is an API, that cannot read less bytes, than was requested, > > > > let's move atusb driver to use it. It will fix all potintial bugs with > > > > uninit values and make code more modern > > > > > > > > Fail log: > > > > > > > > BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > > > > BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > > > > BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > > > > Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd > > > > ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] > > > > atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] > > > > atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 > > > > usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 > > > > > > > > Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") > > > > Cc: stable@vger.kernel.org # 5.9 > > > > Reported-by: Alexander Potapenko <glider@google.com> > > > > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > > > > --- > > > > drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- > > > > 1 file changed, 38 insertions(+), 23 deletions(-) > > > > > > > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > > > > index 23ee0b14cbfa..43befea0110f 100644 > > > > --- a/drivers/net/ieee802154/atusb.c > > > > +++ b/drivers/net/ieee802154/atusb.c > > > > @@ -80,10 +80,9 @@ struct atusb_chip_data { > > > > * in atusb->err and reject all subsequent requests until the error is cleared. > > > > */ > > > > > > > > -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, > > > > - __u8 request, __u8 requesttype, > > > > - __u16 value, __u16 index, > > > > - void *data, __u16 size, int timeout) > > > > +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, > > > > + __u16 value, __u16 index, > > > > + void *data, __u16 size, int timeout) > > > > > > Why do you need a wrapper function at all? Why not just call the real > > > usb functions instead? > > > ... > > > > > > > I would recommend just moving to use the real USB functions and no > > > wrapper function at all like this, it will make things more obvious and > > > easier to understand over time. > > > > okay. > > With the small fix handle the actual KASAN report applied now It was? What is the git commit id? thanks, greg k-h
Hello. On 05.01.22 09:08, Greg KH wrote: > On Tue, Jan 04, 2022 at 08:41:23PM +0100, Stefan Schmidt wrote: >> Hello. >> >> On 03.01.22 16:35, Alexander Aring wrote: >>> Hi, >>> >>> On Mon, 3 Jan 2022 at 08:03, Greg KH <greg@kroah.com> wrote: >>>> >>>> On Sun, Jan 02, 2022 at 08:19:43PM +0300, Pavel Skripkin wrote: >>>>> Alexander reported a use of uninitialized value in >>>>> atusb_set_extended_addr(), that is caused by reading 0 bytes via >>>>> usb_control_msg(). >>>>> >>>>> Since there is an API, that cannot read less bytes, than was requested, >>>>> let's move atusb driver to use it. It will fix all potintial bugs with >>>>> uninit values and make code more modern >>>>> >>>>> Fail log: >>>>> >>>>> BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] >>>>> BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] >>>>> BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 >>>>> Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd >>>>> ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] >>>>> atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] >>>>> atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 >>>>> usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 >>>>> >>>>> Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") >>>>> Cc: stable@vger.kernel.org # 5.9 >>>>> Reported-by: Alexander Potapenko <glider@google.com> >>>>> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >>>>> --- >>>>> drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- >>>>> 1 file changed, 38 insertions(+), 23 deletions(-) >>>>> >>>>> diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c >>>>> index 23ee0b14cbfa..43befea0110f 100644 >>>>> --- a/drivers/net/ieee802154/atusb.c >>>>> +++ b/drivers/net/ieee802154/atusb.c >>>>> @@ -80,10 +80,9 @@ struct atusb_chip_data { >>>>> * in atusb->err and reject all subsequent requests until the error is cleared. >>>>> */ >>>>> >>>>> -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, >>>>> - __u8 request, __u8 requesttype, >>>>> - __u16 value, __u16 index, >>>>> - void *data, __u16 size, int timeout) >>>>> +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, >>>>> + __u16 value, __u16 index, >>>>> + void *data, __u16 size, int timeout) >>>> >>>> Why do you need a wrapper function at all? Why not just call the real >>>> usb functions instead? >> >>> ... >> >>>> >>>> I would recommend just moving to use the real USB functions and no >>>> wrapper function at all like this, it will make things more obvious and >>>> easier to understand over time. >>> >>> okay. >> >> With the small fix handle the actual KASAN report applied now > > It was? What is the git commit id? I applied it to my wpan tree from where it will go to the net tree with my next pull request. https://git.kernel.org/pub/scm/linux/kernel/git/sschmidt/wpan.git/commit/?id=754e4382354f7908923a1949d8dc8d05f82f09cb regards Stefan Schmidt
diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 23ee0b14cbfa..43befea0110f 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -80,10 +80,9 @@ struct atusb_chip_data { * in atusb->err and reject all subsequent requests until the error is cleared. */ -static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, - __u8 request, __u8 requesttype, - __u16 value, __u16 index, - void *data, __u16 size, int timeout) +static int atusb_control_msg_recv(struct atusb *atusb, __u8 request, __u8 requesttype, + __u16 value, __u16 index, + void *data, __u16 size, int timeout) { struct usb_device *usb_dev = atusb->usb_dev; int ret; @@ -91,8 +90,30 @@ static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, if (atusb->err) return atusb->err; - ret = usb_control_msg(usb_dev, pipe, request, requesttype, - value, index, data, size, timeout); + ret = usb_control_msg_recv(usb_dev, 0, request, requesttype, + value, index, data, size, timeout, GFP_KERNEL); + if (ret < 0) { + atusb->err = ret; + dev_err(&usb_dev->dev, + "%s: req 0x%02x val 0x%x idx 0x%x, error %d\n", + __func__, request, value, index, ret); + } + + return ret; +} + +static int atusb_control_msg_send(struct atusb *atusb, __u8 request, __u8 requesttype, + __u16 value, __u16 index, + void *data, __u16 size, int timeout) +{ + struct usb_device *usb_dev = atusb->usb_dev; + int ret; + + if (atusb->err) + return atusb->err; + + ret = usb_control_msg_send(usb_dev, 0, request, requesttype, + value, index, data, size, timeout, GFP_KERNEL); if (ret < 0) { atusb->err = ret; dev_err(&usb_dev->dev, @@ -107,8 +128,7 @@ static int atusb_command(struct atusb *atusb, u8 cmd, u8 arg) struct usb_device *usb_dev = atusb->usb_dev; dev_dbg(&usb_dev->dev, "%s: cmd = 0x%x\n", __func__, cmd); - return atusb_control_msg(atusb, usb_sndctrlpipe(usb_dev, 0), - cmd, ATUSB_REQ_TO_DEV, arg, 0, NULL, 0, 1000); + return atusb_control_msg_send(atusb, cmd, ATUSB_REQ_TO_DEV, arg, 0, NULL, 0, 1000); } static int atusb_write_reg(struct atusb *atusb, u8 reg, u8 value) @@ -116,9 +136,8 @@ static int atusb_write_reg(struct atusb *atusb, u8 reg, u8 value) struct usb_device *usb_dev = atusb->usb_dev; dev_dbg(&usb_dev->dev, "%s: 0x%02x <- 0x%02x\n", __func__, reg, value); - return atusb_control_msg(atusb, usb_sndctrlpipe(usb_dev, 0), - ATUSB_REG_WRITE, ATUSB_REQ_TO_DEV, - value, reg, NULL, 0, 1000); + return atusb_control_msg_send(atusb, ATUSB_REG_WRITE, ATUSB_REQ_TO_DEV, + value, reg, NULL, 0, 1000); } static int atusb_read_reg(struct atusb *atusb, u8 reg) @@ -133,9 +152,8 @@ static int atusb_read_reg(struct atusb *atusb, u8 reg) return -ENOMEM; dev_dbg(&usb_dev->dev, "%s: reg = 0x%x\n", __func__, reg); - ret = atusb_control_msg(atusb, usb_rcvctrlpipe(usb_dev, 0), - ATUSB_REG_READ, ATUSB_REQ_FROM_DEV, - 0, reg, buffer, 1, 1000); + ret = atusb_control_msg_recv(atusb, ATUSB_REG_READ, ATUSB_REQ_FROM_DEV, + 0, reg, buffer, 1, 1000); if (ret >= 0) { value = buffer[0]; @@ -805,9 +823,8 @@ static int atusb_get_and_show_revision(struct atusb *atusb) return -ENOMEM; /* Get a couple of the ATMega Firmware values */ - ret = atusb_control_msg(atusb, usb_rcvctrlpipe(usb_dev, 0), - ATUSB_ID, ATUSB_REQ_FROM_DEV, 0, 0, - buffer, 3, 1000); + ret = atusb_control_msg_recv(atusb, ATUSB_ID, ATUSB_REQ_FROM_DEV, 0, 0, + buffer, 3, 1000); if (ret >= 0) { atusb->fw_ver_maj = buffer[0]; atusb->fw_ver_min = buffer[1]; @@ -861,9 +878,8 @@ static int atusb_get_and_show_build(struct atusb *atusb) if (!build) return -ENOMEM; - ret = atusb_control_msg(atusb, usb_rcvctrlpipe(usb_dev, 0), - ATUSB_BUILD, ATUSB_REQ_FROM_DEV, 0, 0, - build, ATUSB_BUILD_SIZE, 1000); + ret = atusb_control_msg_recv(atusb, ATUSB_BUILD, ATUSB_REQ_FROM_DEV, 0, 0, + build, ATUSB_BUILD_SIZE, 1000); if (ret >= 0) { build[ret] = 0; dev_info(&usb_dev->dev, "Firmware: build %s\n", build); @@ -985,9 +1001,8 @@ static int atusb_set_extended_addr(struct atusb *atusb) return -ENOMEM; /* Firmware is new enough so we fetch the address from EEPROM */ - ret = atusb_control_msg(atusb, usb_rcvctrlpipe(usb_dev, 0), - ATUSB_EUI64_READ, ATUSB_REQ_FROM_DEV, 0, 0, - buffer, IEEE802154_EXTENDED_ADDR_LEN, 1000); + ret = atusb_control_msg_recv(atusb, ATUSB_EUI64_READ, ATUSB_REQ_FROM_DEV, 0, 0, + buffer, IEEE802154_EXTENDED_ADDR_LEN, 1000); if (ret < 0) { dev_err(&usb_dev->dev, "failed to fetch extended address, random address set\n"); ieee802154_random_extended_addr(&atusb->hw->phy->perm_extended_addr);
Alexander reported a use of uninitialized value in atusb_set_extended_addr(), that is caused by reading 0 bytes via usb_control_msg(). Since there is an API, that cannot read less bytes, than was requested, let's move atusb driver to use it. It will fix all potintial bugs with uninit values and make code more modern Fail log: BUG: KASAN: uninit-cmp in ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] BUG: KASAN: uninit-cmp in atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] BUG: KASAN: uninit-cmp in atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 Uninit value used in comparison: 311daa649a2003bd stack handle: 000000009a2003bd ieee802154_is_valid_extended_unicast_addr include/linux/ieee802154.h:310 [inline] atusb_set_extended_addr drivers/net/ieee802154/atusb.c:1000 [inline] atusb_probe.cold+0x29f/0x14db drivers/net/ieee802154/atusb.c:1056 usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 Fixes: 7490b008d123 ("ieee802154: add support for atusb transceiver") Cc: stable@vger.kernel.org # 5.9 Reported-by: Alexander Potapenko <glider@google.com> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 23 deletions(-)