* Use of uninitialized value in atusb_set_extended_addr() @ 2021-12-24 19:24 Alexander Potapenko 2021-12-24 19:57 ` Pavel Skripkin 2022-01-02 17:19 ` [PATCH RFT] ieee802154: atusb: move to new USB API Pavel Skripkin 0 siblings, 2 replies; 21+ messages in thread From: Alexander Potapenko @ 2021-12-24 19:24 UTC (permalink / raw) To: David Miller, stefan; +Cc: paskripkin, linux-wpan, syzkaller-bugs Hi folks, KASAN* reported a use of uninitialized value in atusb_set_extended_addr(). The value came from kmalloc() in the same function, but then apparently atusb_control_msg() returned 0, leaving `buffer` intact but also avoiding the ret < 0 check. Then the buffer got passed to ieee802154_is_valid_extended_unicast_addr(), which used it in a comparison - at that point KASAN reported an error. * - this is an experiment to make KASAN detect some limited subset of bugs caused by using uninitialized values. Full report is below, I am not sure if it's enough to kzalloc the buffer, or we'd better check the return values more carefully. =========================== usb 7-1: ATUSB: AT86RF230 version 187 usb 7-1: Firmware: major: 3, minor: 0, hardware type: ATUSB (0) usb write operation failed. (-71) usb 6-1: dvb_usb_v2: will pass the complete MPEG2 transport stream to the software demuxer dvbdev: DVB: registering new adapter (Technisat CableStar Combo HD CI) usb 6-1: media controller created usb read operation failed. (-71) usb write operation failed. (-71) dvb_usb_az6007: probe of 6-1:0.0 failed with error -5 usb 6-1: USB disconnect, device number 6 usb 7-1: Firmware: build 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 call_driver_probe drivers/base/dd.c:517 [inline] really_probe+0x245/0xcd0 drivers/base/dd.c:596 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427 __device_attach+0x226/0x4a0 drivers/base/dd.c:969 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487 device_add+0xb83/0x1e20 drivers/base/core.c:3394 usb_set_configuration+0x1041/0x19c0 drivers/usb/core/message.c:2170 usb_generic_driver_probe+0xba/0x100 drivers/usb/core/generic.c:238 usb_probe_device+0xd9/0x2c0 drivers/usb/core/driver.c:293 call_driver_probe drivers/base/dd.c:517 [inline] really_probe+0x245/0xcd0 drivers/base/dd.c:596 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427 __device_attach+0x226/0x4a0 drivers/base/dd.c:969 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487 device_add+0xb83/0x1e20 drivers/base/core.c:3394 usb_new_device.cold+0x5f2/0x1018 drivers/usb/core/hub.c:2563 hub_port_connect drivers/usb/core/hub.c:5353 [inline] hub_port_connect_change drivers/usb/core/hub.c:5497 [inline] port_event drivers/usb/core/hub.c:5643 [inline] hub_event+0x247a/0x4470 drivers/usb/core/hub.c:5725 process_one_work+0x976/0x1620 kernel/workqueue.c:2298 worker_thread+0x658/0x11f0 kernel/workqueue.c:2445 kthread+0x405/0x4f0 kernel/kthread.c:327 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 Memory allocated at: kasan_save_stack+0x1e/0x50 mm/kasan/common.c:38 kasan_color_object+0x17/0x80 mm/kasan/color.c:165 __kasan_kmalloc mm/kasan/common.c:529 [inline] __kasan_kmalloc+0xe1/0xf0 mm/kasan/common.c:522 kmalloc include/linux/slab.h:590 [inline] atusb_set_extended_addr drivers/net/ieee802154/atusb.c:983 [inline] atusb_probe+0x991/0x1280 drivers/net/ieee802154/atusb.c:1056 usb_probe_interface+0x314/0x7f0 drivers/usb/core/driver.c:396 call_driver_probe drivers/base/dd.c:517 [inline] really_probe+0x245/0xcd0 drivers/base/dd.c:596 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427 __device_attach+0x226/0x4a0 drivers/base/dd.c:969 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487 device_add+0xb83/0x1e20 drivers/base/core.c:3394 usb_set_configuration+0x1041/0x19c0 drivers/usb/core/message.c:2170 usb_generic_driver_probe+0xba/0x100 drivers/usb/core/generic.c:238 usb_probe_device+0xd9/0x2c0 drivers/usb/core/driver.c:293 call_driver_probe drivers/base/dd.c:517 [inline] really_probe+0x245/0xcd0 drivers/base/dd.c:596 __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:751 driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:781 __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:898 bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427 __device_attach+0x226/0x4a0 drivers/base/dd.c:969 bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487 device_add+0xb83/0x1e20 drivers/base/core.c:3394 usb_new_device.cold+0x5f2/0x1018 drivers/usb/core/hub.c:2563 hub_port_connect drivers/usb/core/hub.c:5353 [inline] hub_port_connect_change drivers/usb/core/hub.c:5497 [inline] port_event drivers/usb/core/hub.c:5643 [inline] hub_event+0x247a/0x4470 drivers/usb/core/hub.c:5725 process_one_work+0x976/0x1620 kernel/workqueue.c:2298 worker_thread+0x658/0x11f0 kernel/workqueue.c:2445 kthread+0x405/0x4f0 kernel/kthread.c:327 ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:295 CPU: 1 PID: 29149 Comm: kworker/1:14 Not tainted 5.16.0-rc6+ #111 Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.14.0-2 04/01/2014 Workqueue: usb_hub_wq hub_event ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: Use of uninitialized value in atusb_set_extended_addr() 2021-12-24 19:24 Use of uninitialized value in atusb_set_extended_addr() Alexander Potapenko @ 2021-12-24 19:57 ` Pavel Skripkin 2022-01-02 17:19 ` [PATCH RFT] ieee802154: atusb: move to new USB API Pavel Skripkin 1 sibling, 0 replies; 21+ messages in thread From: Pavel Skripkin @ 2021-12-24 19:57 UTC (permalink / raw) To: Alexander Potapenko, David Miller, stefan; +Cc: linux-wpan, syzkaller-bugs [-- Attachment #1: Type: text/plain, Size: 942 bytes --] On 12/24/21 22:24, Alexander Potapenko wrote: > Hi folks, > > KASAN* reported a use of uninitialized value in atusb_set_extended_addr(). > The value came from kmalloc() in the same function, but then > apparently atusb_control_msg() returned 0, leaving `buffer` intact but > also avoiding the ret < 0 check. > Then the buffer got passed to > ieee802154_is_valid_extended_unicast_addr(), which used it in a > comparison - at that point KASAN reported an error. > > * - this is an experiment to make KASAN detect some limited subset of > bugs caused by using uninitialized values. > > Full report is below, I am not sure if it's enough to kzalloc the > buffer, or we'd better check the return values more carefully. > Hi Alexander, thanks for your report I think, the right fix is switch to new usb API, since it cannot read less bytes than was requested by caller. Only build-tested, just for thoughts With regards, Pavel Skripkin [-- Attachment #2: test.ph --] [-- Type: text/plain, Size: 4447 bytes --] diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 23ee0b14cbfa..15f387266e18 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,8 +823,7 @@ 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, + 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]; @@ -861,8 +878,7 @@ 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, + 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; @@ -985,8 +1001,7 @@ 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, + 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"); ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH RFT] ieee802154: atusb: move to new USB API 2021-12-24 19:24 Use of uninitialized value in atusb_set_extended_addr() Alexander Potapenko 2021-12-24 19:57 ` Pavel Skripkin @ 2022-01-02 17:19 ` Pavel Skripkin 2022-01-02 22:15 ` Alexander Aring 2022-01-03 13:03 ` Greg KH 1 sibling, 2 replies; 21+ messages in thread From: Pavel Skripkin @ 2022-01-02 17:19 UTC (permalink / raw) To: stefan, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Pavel Skripkin, stable, Alexander Potapenko 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) { 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-02 17:19 ` [PATCH RFT] ieee802154: atusb: move to new USB API Pavel Skripkin @ 2022-01-02 22:15 ` Alexander Aring 2022-01-02 22:21 ` Pavel Skripkin 2022-01-03 13:04 ` [PATCH RFT] ieee802154: atusb: move to new USB API Greg KH 2022-01-03 13:03 ` Greg KH 1 sibling, 2 replies; 21+ messages in thread From: Alexander Aring @ 2022-01-02 22:15 UTC (permalink / raw) To: Pavel Skripkin Cc: Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-02 22:15 ` Alexander Aring @ 2022-01-02 22:21 ` Pavel Skripkin 2022-01-02 22:36 ` Alexander Aring 2022-01-03 13:04 ` [PATCH RFT] ieee802154: atusb: move to new USB API Greg KH 1 sibling, 1 reply; 21+ messages in thread From: Pavel Skripkin @ 2022-01-02 22:21 UTC (permalink / raw) To: Alexander Aring Cc: Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-02 22:21 ` Pavel Skripkin @ 2022-01-02 22:36 ` Alexander Aring 2022-01-03 12:09 ` [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr Pavel Skripkin 0 siblings, 1 reply; 21+ messages in thread From: Alexander Aring @ 2022-01-02 22:36 UTC (permalink / raw) To: Pavel Skripkin Cc: Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-02 22:36 ` Alexander Aring @ 2022-01-03 12:09 ` Pavel Skripkin 2022-01-03 15:27 ` Alexander Aring 2022-01-04 15:40 ` Stefan Schmidt 0 siblings, 2 replies; 21+ messages in thread From: Pavel Skripkin @ 2022-01-03 12:09 UTC (permalink / raw) To: stefan, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Pavel Skripkin, Alexander Potapenko 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> Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- 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 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c index 23ee0b14cbfa..e6cc816dd7a1 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", -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-03 12:09 ` [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr Pavel Skripkin @ 2022-01-03 15:27 ` Alexander Aring 2022-01-04 15:40 ` Stefan Schmidt 1 sibling, 0 replies; 21+ messages in thread From: Alexander Aring @ 2022-01-03 15:27 UTC (permalink / raw) To: Pavel Skripkin Cc: Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, Alexander Potapenko Hi, On Mon, 3 Jan 2022 at 07:09, 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> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> Acked-by: Alexander Aring <aahringo@redhat.com> Although I think if such error exists the most common solution would be to replug the usb device and hope it gets resolved? A retry with a maximum amount would be another try... However, let's hope those errors are rare and we care about them when they occur. Thanks. - Alex ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-03 12:09 ` [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr Pavel Skripkin 2022-01-03 15:27 ` Alexander Aring @ 2022-01-04 15:40 ` Stefan Schmidt 2022-01-04 17:27 ` Pavel Skripkin 1 sibling, 1 reply; 21+ messages in thread From: Stefan Schmidt @ 2022-01-04 15:40 UTC (permalink / raw) To: Pavel Skripkin, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Alexander Potapenko Hello. On 03.01.22 13:09, 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(). > > 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> > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > 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 | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ieee802154/atusb.c b/drivers/net/ieee802154/atusb.c > index 23ee0b14cbfa..e6cc816dd7a1 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", > It compiles, but does not work on the real hardware. [ 1.114698] usb 1-1: new full-speed USB device number 2 using uhci_hcd [ 1.261691] usb 1-1: New USB device found, idVendor=20b7, idProduct=1540, bcdDevice= 0.01 [ 1.263421] usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=1 [ 1.264952] usb 1-1: SerialNumber: 4630333438371502231a [ 1.278042] usb 1-1: ATUSB: AT86RF231 version 2 [ 1.281087] usb 1-1: Firmware: major: 0, minor: 3, hardware type: ATUSB (2) [ 1.285191] usb 1-1: atusb_control_msg: req 0x01 val 0x0 idx 0x0, error -61 [ 1.286903] usb 1-1: failed to fetch extended address, random address set [ 1.288757] usb 1-1: atusb_probe: initialization failed, error = -61 [ 1.290922] atusb: probe of 1-1:1.0 failed with error -61 Without your patch it works as expected: [ 1.091925] usb 1-1: new full-speed USB device number 2 using uhci_hcd [ 1.237743] usb 1-1: New USB device found, idVendor=20b7, idProduct=1540, bcdDevice= 0.01 [ 1.239788] usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=1 [ 1.241432] usb 1-1: SerialNumber: 4630333438371502231a [ 1.255012] usb 1-1: ATUSB: AT86RF231 version 2 [ 1.258073] usb 1-1: Firmware: major: 0, minor: 3, hardware type: ATUSB (2) [ 1.262170] usb 1-1: Firmware: build #132 Mo 28. Nov 16:20:35 CET 2016 [ 1.266195] usb 1-1: Read permanent extended address 10:e2:d5:ff:ff:00:02:e8 from device regards Stefan Schmidt ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 15:40 ` Stefan Schmidt @ 2022-01-04 17:27 ` Pavel Skripkin 2022-01-04 18:04 ` Stefan Schmidt 0 siblings, 1 reply; 21+ messages in thread From: Pavel Skripkin @ 2022-01-04 17:27 UTC (permalink / raw) To: Stefan Schmidt, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Alexander Potapenko On 1/4/22 18:40, Stefan Schmidt wrote: > > It compiles, but does not work on the real hardware. > > [ 1.114698] usb 1-1: new full-speed USB device number 2 using uhci_hcd > [ 1.261691] usb 1-1: New USB device found, idVendor=20b7, > idProduct=1540, bcdDevice= 0.01 > [ 1.263421] usb 1-1: New USB device strings: Mfr=0, Product=0, > SerialNumber=1 > [ 1.264952] usb 1-1: SerialNumber: 4630333438371502231a > [ 1.278042] usb 1-1: ATUSB: AT86RF231 version 2 > [ 1.281087] usb 1-1: Firmware: major: 0, minor: 3, hardware type: > ATUSB (2) > [ 1.285191] usb 1-1: atusb_control_msg: req 0x01 val 0x0 idx 0x0, > error -61 > [ 1.286903] usb 1-1: failed to fetch extended address, random address set > [ 1.288757] usb 1-1: atusb_probe: initialization failed, error = -61 > [ 1.290922] atusb: probe of 1-1:1.0 failed with error -61 > > > Without your patch it works as expected: > > [ 1.091925] usb 1-1: new full-speed USB device number 2 using uhci_hcd > [ 1.237743] usb 1-1: New USB device found, idVendor=20b7, > idProduct=1540, bcdDevice= 0.01 > [ 1.239788] usb 1-1: New USB device strings: Mfr=0, Product=0, > SerialNumber=1 > [ 1.241432] usb 1-1: SerialNumber: 4630333438371502231a > [ 1.255012] usb 1-1: ATUSB: AT86RF231 version 2 > [ 1.258073] usb 1-1: Firmware: major: 0, minor: 3, hardware type: > ATUSB (2) > [ 1.262170] usb 1-1: Firmware: build #132 Mo 28. Nov 16:20:35 CET 2016 > [ 1.266195] usb 1-1: Read permanent extended address > 10:e2:d5:ff:ff:00:02:e8 from device > Hi Stefan, thanks for testing on real hw. It looks like there is corner case, that Greg mentioned in this thread. atusb_get_and_show_build() reads firmware build info, which may have various length. Maybe we can change atusb_control_msg() to usb_control_msg() in atusb_get_and_show_build(), since other callers do not have this problem With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 17:27 ` Pavel Skripkin @ 2022-01-04 18:04 ` Stefan Schmidt 2022-01-04 18:11 ` Pavel Skripkin 2022-01-04 18:28 ` [PATCH v3] " Pavel Skripkin 0 siblings, 2 replies; 21+ messages in thread From: Stefan Schmidt @ 2022-01-04 18:04 UTC (permalink / raw) To: Pavel Skripkin, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Alexander Potapenko Hello. On 04.01.22 18:27, Pavel Skripkin wrote: > On 1/4/22 18:40, Stefan Schmidt wrote: >> >> It compiles, but does not work on the real hardware. >> >> [ 1.114698] usb 1-1: new full-speed USB device number 2 using uhci_hcd >> [ 1.261691] usb 1-1: New USB device found, idVendor=20b7, >> idProduct=1540, bcdDevice= 0.01 >> [ 1.263421] usb 1-1: New USB device strings: Mfr=0, Product=0, >> SerialNumber=1 >> [ 1.264952] usb 1-1: SerialNumber: 4630333438371502231a >> [ 1.278042] usb 1-1: ATUSB: AT86RF231 version 2 >> [ 1.281087] usb 1-1: Firmware: major: 0, minor: 3, hardware type: >> ATUSB (2) >> [ 1.285191] usb 1-1: atusb_control_msg: req 0x01 val 0x0 idx 0x0, >> error -61 >> [ 1.286903] usb 1-1: failed to fetch extended address, random >> address set >> [ 1.288757] usb 1-1: atusb_probe: initialization failed, error = -61 >> [ 1.290922] atusb: probe of 1-1:1.0 failed with error -61 >> >> >> Without your patch it works as expected: >> >> [ 1.091925] usb 1-1: new full-speed USB device number 2 using uhci_hcd >> [ 1.237743] usb 1-1: New USB device found, idVendor=20b7, >> idProduct=1540, bcdDevice= 0.01 >> [ 1.239788] usb 1-1: New USB device strings: Mfr=0, Product=0, >> SerialNumber=1 >> [ 1.241432] usb 1-1: SerialNumber: 4630333438371502231a >> [ 1.255012] usb 1-1: ATUSB: AT86RF231 version 2 >> [ 1.258073] usb 1-1: Firmware: major: 0, minor: 3, hardware type: >> ATUSB (2) >> [ 1.262170] usb 1-1: Firmware: build #132 Mo 28. Nov 16:20:35 CET 2016 >> [ 1.266195] usb 1-1: Read permanent extended address >> 10:e2:d5:ff:ff:00:02:e8 from device >> > > Hi Stefan, > > thanks for testing on real hw. > > It looks like there is corner case, that Greg mentioned in this thread. > atusb_get_and_show_build() reads firmware build info, which may have > various length. > > Maybe we can change atusb_control_msg() to usb_control_msg() in > atusb_get_and_show_build(), since other callers do not have this problem That works for me. I will also have a look at the use of the modern USB API for next. The fix here has a higher prio for me to get in and backported though. Once we have this we can look at bigger changes in atusb. regards Stefan Schmidt ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 18:04 ` Stefan Schmidt @ 2022-01-04 18:11 ` Pavel Skripkin 2022-01-04 18:28 ` [PATCH v3] " Pavel Skripkin 1 sibling, 0 replies; 21+ messages in thread From: Pavel Skripkin @ 2022-01-04 18:11 UTC (permalink / raw) To: Stefan Schmidt, alex.aring, davem, kuba Cc: linux-wpan, netdev, linux-kernel, Alexander Potapenko On 1/4/22 21:04, Stefan Schmidt wrote: >> Hi Stefan, >> >> thanks for testing on real hw. >> >> It looks like there is corner case, that Greg mentioned in this thread. >> atusb_get_and_show_build() reads firmware build info, which may have >> various length. >> >> Maybe we can change atusb_control_msg() to usb_control_msg() in >> atusb_get_and_show_build(), since other callers do not have this problem > > That works for me. > Nice! Will prepare v3. Thanks for testing once again! > I will also have a look at the use of the modern USB API for next. The > fix here has a higher prio for me to get in and backported though. Once > we have this we can look at bigger changes in atusb. > With regards, Pavel Skripkin ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 18:04 ` Stefan Schmidt 2022-01-04 18:11 ` Pavel Skripkin @ 2022-01-04 18:28 ` Pavel Skripkin 2022-01-04 18:57 ` Alexander Aring 1 sibling, 1 reply; 21+ messages in thread From: Pavel Skripkin @ 2022-01-04 18:28 UTC (permalink / raw) To: stefan, alex.aring, davem, kuba, linux-wpan, netdev, linux-kernel Cc: Pavel Skripkin, Alexander Potapenko, Alexander Aring 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(-) 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); -- 2.34.1 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v3] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 18:28 ` [PATCH v3] " Pavel Skripkin @ 2022-01-04 18:57 ` Alexander Aring 2022-01-04 19:25 ` Stefan Schmidt 0 siblings, 1 reply; 21+ messages in thread From: Alexander Aring @ 2022-01-04 18:57 UTC (permalink / raw) To: Pavel Skripkin Cc: stefan, alex.aring, davem, Jakub Kicinski, linux-wpan, Network Development, linux-kernel, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3] ieee802154: atusb: fix uninit value in atusb_set_extended_addr 2022-01-04 18:57 ` Alexander Aring @ 2022-01-04 19:25 ` Stefan Schmidt 0 siblings, 0 replies; 21+ messages in thread From: Stefan Schmidt @ 2022-01-04 19:25 UTC (permalink / raw) To: Alexander Aring, Pavel Skripkin Cc: alex.aring, davem, Jakub Kicinski, linux-wpan, Network Development, linux-kernel, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-02 22:15 ` Alexander Aring 2022-01-02 22:21 ` Pavel Skripkin @ 2022-01-03 13:04 ` Greg KH 1 sibling, 0 replies; 21+ messages in thread From: Greg KH @ 2022-01-03 13:04 UTC (permalink / raw) To: Alexander Aring Cc: Pavel Skripkin, Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-02 17:19 ` [PATCH RFT] ieee802154: atusb: move to new USB API Pavel Skripkin 2022-01-02 22:15 ` Alexander Aring @ 2022-01-03 13:03 ` Greg KH 2022-01-03 15:35 ` Alexander Aring 1 sibling, 1 reply; 21+ messages in thread From: Greg KH @ 2022-01-03 13:03 UTC (permalink / raw) To: Pavel Skripkin Cc: stefan, alex.aring, davem, kuba, linux-wpan, netdev, linux-kernel, stable, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-03 13:03 ` Greg KH @ 2022-01-03 15:35 ` Alexander Aring 2022-01-04 19:41 ` Stefan Schmidt 0 siblings, 1 reply; 21+ messages in thread From: Alexander Aring @ 2022-01-03 15:35 UTC (permalink / raw) To: Greg KH Cc: Pavel Skripkin, Stefan Schmidt, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-03 15:35 ` Alexander Aring @ 2022-01-04 19:41 ` Stefan Schmidt 2022-01-05 8:08 ` Greg KH 0 siblings, 1 reply; 21+ messages in thread From: Stefan Schmidt @ 2022-01-04 19:41 UTC (permalink / raw) To: Alexander Aring, Greg KH Cc: Pavel Skripkin, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-04 19:41 ` Stefan Schmidt @ 2022-01-05 8:08 ` Greg KH 2022-01-05 9:01 ` Stefan Schmidt 0 siblings, 1 reply; 21+ messages in thread From: Greg KH @ 2022-01-05 8:08 UTC (permalink / raw) To: Stefan Schmidt Cc: Alexander Aring, Pavel Skripkin, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH RFT] ieee802154: atusb: move to new USB API 2022-01-05 8:08 ` Greg KH @ 2022-01-05 9:01 ` Stefan Schmidt 0 siblings, 0 replies; 21+ messages in thread From: Stefan Schmidt @ 2022-01-05 9:01 UTC (permalink / raw) To: Greg KH Cc: Alexander Aring, Pavel Skripkin, David S. Miller, Jakub Kicinski, linux-wpan - ML, open list:NETWORKING [GENERAL], kernel list, # 3.19.x, Alexander Potapenko 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-01-05 9:02 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-12-24 19:24 Use of uninitialized value in atusb_set_extended_addr() Alexander Potapenko 2021-12-24 19:57 ` Pavel Skripkin 2022-01-02 17:19 ` [PATCH RFT] ieee802154: atusb: move to new USB API Pavel Skripkin 2022-01-02 22:15 ` Alexander Aring 2022-01-02 22:21 ` Pavel Skripkin 2022-01-02 22:36 ` Alexander Aring 2022-01-03 12:09 ` [PATCH v2] ieee802154: atusb: fix uninit value in atusb_set_extended_addr Pavel Skripkin 2022-01-03 15:27 ` Alexander Aring 2022-01-04 15:40 ` Stefan Schmidt 2022-01-04 17:27 ` Pavel Skripkin 2022-01-04 18:04 ` Stefan Schmidt 2022-01-04 18:11 ` Pavel Skripkin 2022-01-04 18:28 ` [PATCH v3] " Pavel Skripkin 2022-01-04 18:57 ` Alexander Aring 2022-01-04 19:25 ` Stefan Schmidt 2022-01-03 13:04 ` [PATCH RFT] ieee802154: atusb: move to new USB API Greg KH 2022-01-03 13:03 ` Greg KH 2022-01-03 15:35 ` Alexander Aring 2022-01-04 19:41 ` Stefan Schmidt 2022-01-05 8:08 ` Greg KH 2022-01-05 9:01 ` Stefan Schmidt
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.