Use of uninitialized value in atusb_set_extended_addr()

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.

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

Hi,

Quoted 5 lines · Pavel Skripkin

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?

Quoted 4 lines

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.

Quoted 17 lines · Alexander Aring

Hi,

Quoted 5 lines · Pavel Skripkin

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?

Quoted 4 lines

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

Quoted 5 lines

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:

(quoted diff) +2 -2

-       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

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 <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

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 <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---

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(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..e6cc816dd7a1 100644

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

Quoted 21 lines · Stefan Schmidt

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:

Quoted 13 lines

[    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

Quoted 7 lines

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.

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 <[email protected]>

Acked-by Alexander Aring <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---

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

Hi,

Quoted 26 lines · Pavel Skripkin

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 <[email protected]>

Acked-by Alexander Aring <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---

Quoted 5 lines

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.

Hi,

Quoted 5 lines · Pavel Skripkin

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?

Quoted 4 lines

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.

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 [email protected] # 5.9

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---
 drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 23 deletions(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..43befea0110f 100644

drivers/net/ieee802154/atusb.c +0 -0

 {
 	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;
 

drivers/net/ieee802154/atusb.c +17 -2

+
+	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)

Quoted 35 lines · Pavel Skripkin

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 [email protected] # 5.9

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---
 drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++-------------
 1 file changed, 38 insertions(+), 23 deletions(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..43befea0110f 100644
@@ -80,10 +80,9 @@ struct atusb_chip_data {
  * in atusb->err and reject all subsequent requests until the error is cleared.
  */
 

drivers/net/ieee802154/atusb.c struct atusb_chip_data { · line 85

Quoted 7 lines

drivers/net/ieee802154/atusb.c +3 -4

-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.

Hi,

Quoted 37 lines · Greg KH

Quoted 35 lines · Pavel Skripkin

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 [email protected] # 5.9

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---
  drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++-------------
  1 file changed, 38 insertions(+), 23 deletions(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..43befea0110f 100644
@@ -80,10 +80,9 @@ struct atusb_chip_data {
   * in atusb->err and reject all subsequent requests until the error is cleared.
   */
 

Quoted 10 lines

drivers/net/ieee802154/atusb.c struct atusb_chip_data { · line 85

Quoted 7 lines

drivers/net/ieee802154/atusb.c +3 -4

-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?

...

Quoted 4 lines

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.

Hello.

Quoted 54 lines · Alexander Aring

Hi,

Quoted 47 lines · Greg KH

Quoted 32 lines · Pavel Skripkin

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 [email protected] # 5.9

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---
  drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++-------------
  1 file changed, 38 insertions(+), 23 deletions(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..43befea0110f 100644

drivers/net/ieee802154/atusb.c struct atusb_chip_data { · line 85

Quoted 10 lines

drivers/net/ieee802154/atusb.c +3 -4

@@ -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?

...

Quoted 1 line

Quoted 5 lines

Quoted 3 lines

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

Quoted 61 lines · Stefan Schmidt

Hello.

Quoted 58 lines · Alexander Aring

Hi,

Quoted 47 lines · Greg KH

Quoted 32 lines · Pavel Skripkin

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 [email protected] # 5.9

Reported-by Alexander Potapenko <[email protected]>

Signed-off-by Pavel Skripkin <[email protected]>

---
   drivers/net/ieee802154/atusb.c | 61 +++++++++++++++++++++-------------
   1 file changed, 38 insertions(+), 23 deletions(-)

drivers/net/ieee802154/atusb.c +0 -0

index 23ee0b14cbfa..43befea0110f 100644

drivers/net/ieee802154/atusb.c struct atusb_chip_data { · line 85

Quoted 10 lines

drivers/net/ieee802154/atusb.c +3 -4

@@ -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?

...

Quoted 4 lines

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.

Quoted 3 lines

Quoted 1 line

okay.

With the small fix handle the actual KASAN report applied now

It was? What is the git commit id?