| Message ID | 20220104182806.7188-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v3] ieee802154: atusb: fix uninit value in atusb_set_extended_addr | expand |
Hi, On Tue, Jan 4, 2022 at 1:28 PM 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(). > > Fix it by validating if the number of bytes transferred is actually > correct, since usb_control_msg() may read less bytes, than was requested > by caller. > > 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") > Reported-by: Alexander Potapenko <glider@google.com> > Acked-by: Alexander Aring <aahringo@redhat.com> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > Changes in v3: > - Changed atusb_control_msg() to usb_control_msg() in > atusb_get_and_show_build(), since request there may read various length > data > Thanks for catching this. - Alex
Hello. On 04.01.22 19:57, Alexander Aring wrote: > Hi, > > On Tue, Jan 4, 2022 at 1:28 PM 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(). >> >> Fix it by validating if the number of bytes transferred is actually >> correct, since usb_control_msg() may read less bytes, than was requested >> by caller. >> >> 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") >> Reported-by: Alexander Potapenko <glider@google.com> >> Acked-by: Alexander Aring <aahringo@redhat.com> >> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> >> --- >> >> Changes in v3: >> - Changed atusb_control_msg() to usb_control_msg() in >> atusb_get_and_show_build(), since request there may read various length >> data >> > > Thanks for catching this. Test passed my testing. This patch has been applied to the wpan tree and will be part of the next pull request to net. Thanks! regards Stefan Schmidt
diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 23ee0b14cbfa..2f5e7b31032a 100644 --- a/drivers/net/ieee802154/atusb.c +++ b/drivers/net/ieee802154/atusb.c @@ -93,7 +93,9 @@ static int atusb_control_msg(struct atusb *atusb, unsigned int pipe, ret = usb_control_msg(usb_dev, pipe, request, requesttype, value, index, data, size, timeout); - if (ret < 0) { + if (ret < size) { + ret = ret < 0 ? ret : -ENODATA; + atusb->err = ret; dev_err(&usb_dev->dev, "%s: req 0x%02x val 0x%x idx 0x%x, error %d\n", @@ -861,9 +863,9 @@ 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); + /* We cannot call atusb_control_msg() here, since this request may read various length data */ + ret = usb_control_msg(atusb->usb_dev, usb_rcvctrlpipe(usb_dev, 0), 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);
Alexander reported a use of uninitialized value in atusb_set_extended_addr(), that is caused by reading 0 bytes via usb_control_msg(). Fix it by validating if the number of bytes transferred is actually correct, since usb_control_msg() may read less bytes, than was requested by caller. 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") Reported-by: Alexander Potapenko <glider@google.com> Acked-by: Alexander Aring <aahringo@redhat.com> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Changes in v3: - Changed atusb_control_msg() to usb_control_msg() in atusb_get_and_show_build(), since request there may read various length data Changes in v2: - Reworked fix approach, since moving to new USB API is not suitable for backporting to stable kernels --- drivers/net/ieee802154/atusb.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)