* [PATCH] btusb: fix overflow return values
@ 2013-07-04 12:43 Adam Lee
[not found] ` <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com>
2013-07-08 18:50 ` Marcel Holtmann
0 siblings, 2 replies; 11+ messages in thread
From: Adam Lee @ 2013-07-04 12:43 UTC (permalink / raw)
To: linux-bluetooth
Cc: Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An,
Anthony Wong',
Marcel Holtmann, Gustavo Padovan, Johan Hedberg, open list
PTR_ERR() returns a long type value, but btusb_setup_intel() and
btusb_setup_intel_patching() should return an int type value.
This bug makes the judgement "if (ret < 0)" not working on x86_64
architecture systems, leading to failure as below, even panic.
[ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
[ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
[ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
[ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
[ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
[ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
[ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
[ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
[ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
[ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110)
[ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
[ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110)
[ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
[ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110)
[ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
[ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110)
[ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
[ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110)
[ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000]
For not affecting other modules, I choose to modify the return values
but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s
return types. This is harmless, because the return values were only
used to comparing number 0.
Signed-off-by: Adam Lee <adam.lee@canonical.com>
---
drivers/bluetooth/btusb.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7a7e5f8..0fb2799 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev *hdev,
if (IS_ERR(skb)) {
BT_ERR("%s sending Intel patch command (0x%4.4x) failed (%ld)",
hdev->name, cmd->opcode, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
/* It ensures that the returned event matches the event data read from
@@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s sending initial HCI reset command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s reading Intel fw version command failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
if (skb->len != sizeof(*ver)) {
@@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
BT_ERR("%s entering Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
release_firmware(fw);
- return -PTR_ERR(skb);
+ return -EFAULT;
}
if (skb->data[0]) {
@@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev)
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1289,7 +1289,7 @@ exit_mfg_disable:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
@@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
if (IS_ERR(skb)) {
BT_ERR("%s exiting Intel manufacturer mode failed (%ld)",
hdev->name, PTR_ERR(skb));
- return -PTR_ERR(skb);
+ return -EFAULT;
}
kfree_skb(skb);
--
1.8.3.2
^ permalink raw reply related [flat|nested] 11+ messages in thread[parent not found: <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com>]
* Re: [PATCH] btusb: fix overflow return values [not found] ` <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com> @ 2013-07-05 2:53 ` Yang Bai 2013-07-05 2:59 ` Adam Lee 1 sibling, 0 replies; 11+ messages in thread From: Yang Bai @ 2013-07-05 2:53 UTC (permalink / raw) To: Adam Lee Cc: linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong', Marcel Holtmann, Gustavo Padovan, Johan Hedberg, open list Resend without HTML format. The return value of btusb_setup_intel is compared with 0. Code as: drivers/bluetooth/btusb.c: static int btusb_probe(struct usb_interface *intf, const struct usb_device_id *id) { if (id->driver_info & BTUSB_INTEL) hdev->setup = btusb_setup_intel; } net/bluetooth/hci_core.c: int hci_dev_open(__u16 dev) { if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags)) ret = hdev->setup(hdev); if (!ret) { ... } } Thanks, Yang On Fri, Jul 5, 2013 at 10:37 AM, Yang Bai <hamo.by@gmail.com> wrote: > The return value of btusb_setup_intel is compared with 0. Code as: > > drivers/bluetooth/btusb.c: > static int btusb_probe(struct usb_interface *intf, > const struct usb_device_id *id) > if (id->driver_info & BTUSB_INTEL) > hdev->setup = btusb_setup_intel; > > net/bluetooth/hci_core.c: > int hci_dev_open(__u16 dev) > if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags)) > ret = hdev->setup(hdev); > > if (!ret) { > > > Thanks, > Yang > > > > On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <adam.lee@canonical.com> wrote: >> >> PTR_ERR() returns a long type value, but btusb_setup_intel() and >> btusb_setup_intel_patching() should return an int type value. >> >> This bug makes the judgement "if (ret < 0)" not working on x86_64 >> architecture systems, leading to failure as below, even panic. >> >> [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout >> [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed >> (-110) >> [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout >> [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed >> (-110) >> [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout >> [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed >> (-110) >> [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout >> [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed >> (-110) >> [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout >> [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed >> (-110) >> [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp >> 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000] >> >> For not affecting other modules, I choose to modify the return values >> but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s >> return types. This is harmless, because the return values were only >> used to comparing number 0. >> >> Signed-off-by: Adam Lee <adam.lee@canonical.com> >> --- >> drivers/bluetooth/btusb.c | 14 +++++++------- >> 1 file changed, 7 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c >> index 7a7e5f8..0fb2799 100644 >> --- a/drivers/bluetooth/btusb.c >> +++ b/drivers/bluetooth/btusb.c >> @@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev >> *hdev, >> if (IS_ERR(skb)) { >> BT_ERR("%s sending Intel patch command (0x%4.4x) failed >> (%ld)", >> hdev->name, cmd->opcode, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> >> /* It ensures that the returned event matches the event data read >> from >> @@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) >> if (IS_ERR(skb)) { >> BT_ERR("%s sending initial HCI reset command failed >> (%ld)", >> hdev->name, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> kfree_skb(skb); >> >> @@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) >> if (IS_ERR(skb)) { >> BT_ERR("%s reading Intel fw version command failed (%ld)", >> hdev->name, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> >> if (skb->len != sizeof(*ver)) { >> @@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) >> BT_ERR("%s entering Intel manufacturer mode failed (%ld)", >> hdev->name, PTR_ERR(skb)); >> release_firmware(fw); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> >> if (skb->data[0]) { >> @@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) >> if (IS_ERR(skb)) { >> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", >> hdev->name, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> kfree_skb(skb); >> >> @@ -1289,7 +1289,7 @@ exit_mfg_disable: >> if (IS_ERR(skb)) { >> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", >> hdev->name, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> kfree_skb(skb); >> >> @@ -1307,7 +1307,7 @@ exit_mfg_deactivate: >> if (IS_ERR(skb)) { >> BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", >> hdev->name, PTR_ERR(skb)); >> - return -PTR_ERR(skb); >> + return -EFAULT; >> } >> kfree_skb(skb); >> >> -- >> 1.8.3.2 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ > > > > > -- > """ > Keep It Simple,Stupid. > """ > > Chinese Name: 白杨 > Nick Name: Hamo > Homepage: http://hamobai.com/ > GPG KEY ID: 0xA4691A33 > Key fingerprint = 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33 -- """ Keep It Simple,Stupid. """ Chinese Name: 白杨 Nick Name: Hamo Homepage: http://hamobai.com/ GPG KEY ID: 0xA4691A33 Key fingerprint = 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values [not found] ` <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com> 2013-07-05 2:53 ` Yang Bai @ 2013-07-05 2:59 ` Adam Lee 2013-07-05 4:41 ` Adam Lee 1 sibling, 1 reply; 11+ messages in thread From: Adam Lee @ 2013-07-05 2:59 UTC (permalink / raw) To: Yang Bai Cc: linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong', Marcel Holtmann, Gustavo Padovan, Johan Hedberg, open list On Fri, Jul 05, 2013 at 10:37:07AM +0800, Yang Bai wrote: > The return value of btusb_setup_intel is compared with 0. Code as: > > drivers/bluetooth/btusb.c: > static int btusb_probe(struct usb_interface *intf, > const struct usb_device_id *id) > if (id->driver_info & BTUSB_INTEL) > hdev->setup = btusb_setup_intel; > > net/bluetooth/hci_core.c: > int hci_dev_open(__u16 dev) > if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags)) > ret = hdev->setup(hdev); > > if (!ret) { Yes, for btusb_setup_intel(), the return value is compared with number "0", doesn't break the judgement. But it still overflows stack without this fix. > On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <adam.lee@canonical.com> wrote: > > PTR_ERR() returns a long type value, but btusb_setup_intel() and > btusb_setup_intel_patching() should return an int type value. > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > architecture systems, leading to failure as below, even panic. -- Regards, Adam Lee Hardware Enablement ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values 2013-07-05 2:59 ` Adam Lee @ 2013-07-05 4:41 ` Adam Lee 0 siblings, 0 replies; 11+ messages in thread From: Adam Lee @ 2013-07-05 4:41 UTC (permalink / raw) To: Yang Bai, linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong', Marcel Holtmann, Gustavo Padovan, Johan Hedberg, open list On Fri, Jul 05, 2013 at 10:59:47AM +0800, Adam Lee wrote: > On Fri, Jul 05, 2013 at 10:37:07AM +0800, Yang Bai wrote: > > The return value of btusb_setup_intel is compared with 0. Code as: > > > > drivers/bluetooth/btusb.c: > > static int btusb_probe(struct usb_interface *intf, > > const struct usb_device_id *id) > > if (id->driver_info & BTUSB_INTEL) > > hdev->setup = btusb_setup_intel; > > > > net/bluetooth/hci_core.c: > > int hci_dev_open(__u16 dev) > > if (hdev->setup && test_bit(HCI_SETUP, &hdev->dev_flags)) > > ret = hdev->setup(hdev); > > > > if (!ret) { > > Yes, for btusb_setup_intel(), the return value is compared with number > "0", doesn't break the judgement. > > But it still overflows stack without this fix. And what if the lower 32bits of PTR_ERR() are all "0"? :D > > On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee <adam.lee@canonical.com> wrote: > > > > PTR_ERR() returns a long type value, but btusb_setup_intel() and > > btusb_setup_intel_patching() should return an int type value. > > > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > > architecture systems, leading to failure as below, even panic. -- Regards, Adam Lee Hardware Enablement ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values 2013-07-04 12:43 [PATCH] btusb: fix overflow return values Adam Lee [not found] ` <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com> @ 2013-07-08 18:50 ` Marcel Holtmann 2013-07-09 2:55 ` Adam Lee 1 sibling, 1 reply; 11+ messages in thread From: Marcel Holtmann @ 2013-07-08 18:50 UTC (permalink / raw) To: Adam Lee Cc: linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong', Gustavo Padovan, Johan Hedberg, open list Hi Adam, > PTR_ERR() returns a long type value, but btusb_setup_intel() and > btusb_setup_intel_patching() should return an int type value. > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > architecture systems, leading to failure as below, even panic. > > [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout > [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout > [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout > [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout > [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout > [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout > [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout > [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout > [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout > [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout > [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110) > [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout > [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110) > [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout > [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110) > [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout > [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110) > [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000] > > For not affecting other modules, I choose to modify the return values > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s > return types. This is harmless, because the return values were only > used to comparing number 0. there are tons of examples in various subsystems and drivers where we return PTR_ERR from a function calls returning int. So I wonder what is actually going wrong here. If this is x86_64 specific problem with PTR_ERR vs int, then we should have this problem everywhere in the kernel. Regards Marcel ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values 2013-07-08 18:50 ` Marcel Holtmann @ 2013-07-09 2:55 ` Adam Lee 2013-07-09 7:40 ` Adam Lee 2013-07-09 15:32 ` [PATCH] btusb: fix overflow return values Gustavo Padovan 0 siblings, 2 replies; 11+ messages in thread From: Adam Lee @ 2013-07-09 2:55 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Gustavo Padovan, Johan Hedberg, open list [-- Attachment #1: Type: text/plain, Size: 1605 bytes --] On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote: > Hi Adam, > > > PTR_ERR() returns a long type value, but btusb_setup_intel() and > > btusb_setup_intel_patching() should return an int type value. > > > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > > architecture systems, leading to failure as below, even panic. > > ... > > For not affecting other modules, I choose to modify the return values > > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s > > return types. This is harmless, because the return values were only > > used to comparing number 0. > > there are tons of examples in various subsystems and drivers where we > return PTR_ERR from a function calls returning int. > > So I wonder what is actually going wrong here. If this is x86_64 > specific problem with PTR_ERR vs int, then we should have this problem > everywhere in the kernel. Hi, Marcel I see you point, the difference between here and other subsystems are: 1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I checked. 2, the judgement is "if (ret < 0)" here but other places are "if (ret)". I'm not saying other subsystems are 100% right, but here, returning -PTR_ERR() and checking "if (ret < 0)" make the judgement broken much much more easily. I attached a testing C file, run it on x86_64, you will see the bug. PS, about other subsystems, I also think returning PTR_ERR() from a function calls returning int considered harmful sometimes, will talk about that in other thread. Great thanks. -- Regards, Adam Lee Hardware Enablement [-- Attachment #2: ptr_err.c --] [-- Type: text/x-csrc, Size: 606 bytes --] #include <stdio.h> static inline long PTR_ERR(const void *ptr) { return (long) ptr; } int main(int argc, const char *argv[]) { printf("sizeof(char) = %lu, sizeof(int) = %lu, sizeof(long) = %lu\n\n", sizeof(char), sizeof(int), sizeof(long)); /*This address is in kernel space, check Documentation/x86/x86_64/mm.txt*/ void *ptr = (void *)0Xffff8900f0000000; printf("ptr = %p, PTR_ERR(ptr) = %lx, (int)(-PTR_ERR(ptr)) = %d\n\n", ptr, PTR_ERR(ptr), (int)(-PTR_ERR(ptr))); if ((int)(-PTR_ERR(ptr)) < 0) printf("That's what the codes want.\n"); else printf("Bug happens!\n"); return 0; } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values 2013-07-09 2:55 ` Adam Lee @ 2013-07-09 7:40 ` Adam Lee 2013-07-09 8:48 ` [PATCH v2] btusb: fix wrong use of PTR_ERR() Adam Lee 2013-07-09 15:32 ` [PATCH] btusb: fix overflow return values Gustavo Padovan 1 sibling, 1 reply; 11+ messages in thread From: Adam Lee @ 2013-07-09 7:40 UTC (permalink / raw) To: Marcel Holtmann Cc: linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Gustavo Padovan, Johan Hedberg, open list On Tue, Jul 09, 2013 at 10:55:01AM +0800, Adam Lee wrote: > On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote: > > Hi Adam, > > > > > PTR_ERR() returns a long type value, but btusb_setup_intel() and > > > btusb_setup_intel_patching() should return an int type value. > > > > > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > > > architecture systems, leading to failure as below, even panic. > > > ... > > > For not affecting other modules, I choose to modify the return values > > > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s > > > return types. This is harmless, because the return values were only > > > used to comparing number 0. > > > > there are tons of examples in various subsystems and drivers where we > > return PTR_ERR from a function calls returning int. > > > > So I wonder what is actually going wrong here. If this is x86_64 > > specific problem with PTR_ERR vs int, then we should have this problem > > everywhere in the kernel. > > Hi, Marcel > > I see you point, the difference between here and other subsystems are: > > 1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I > checked. > 2, the judgement is "if (ret < 0)" here but other places are "if (ret)". > > I'm not saying other subsystems are 100% right, but here, returning > -PTR_ERR() and checking "if (ret < 0)" make the judgement broken much > much more easily. > > I attached a testing C file, run it on x86_64, you will see the bug. > > PS, about other subsystems, I also think returning PTR_ERR() from a > function calls returning int considered harmful sometimes, will talk > about that in other thread. Hi, all After diving into the err.h, I realized this patch contains some modifications which are actually not necessary. Will submit a v2 version and explain. PS, other subsystems are using it right, this not. -- Regards, Adam Lee Hardware Enablement ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2] btusb: fix wrong use of PTR_ERR() 2013-07-09 7:40 ` Adam Lee @ 2013-07-09 8:48 ` Adam Lee 0 siblings, 0 replies; 11+ messages in thread From: Adam Lee @ 2013-07-09 8:48 UTC (permalink / raw) To: linux-bluetooth Cc: Marcel Holtmann, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Gustavo Padovan, Johan Hedberg, open list [-- Attachment #1: Type: text/plain, Size: 2674 bytes --] PTR_ERR() returns a signed long type value which is limited by IS_ERR(), it must be a negative number and bigger than -MAX_ERRNO(-4095). The bug here, btusb_setup_intel_patching()'s return value might come from -PTR_ERR() which must be a positive number, we can't judge if it's an error by "if (ret < 0)", the wrong use here leads to failure as below, even panic. Error log: [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110) [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110) [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110) [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110) [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000] Signed-off-by: Adam Lee <adam.lee@canonical.com> --- drivers/bluetooth/btusb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 7a7e5f8..05a38a2 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -1256,7 +1256,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) ret = btusb_setup_intel_patching(hdev, fw, &fw_ptr, &disable_patch); - if (ret < 0) + if (!ret) goto exit_mfg_deactivate; } -- 1.8.3.2 [-- Attachment #2: ptr_err.c --] [-- Type: text/x-csrc, Size: 551 bytes --] #include <stdio.h> #define MAX_ERRNO 4095 #define IS_ERR_VALUE(x) ((x) >= (unsigned long)-MAX_ERRNO) static inline long IS_ERR(const void *ptr) { return IS_ERR_VALUE((unsigned long)ptr); } static inline long PTR_ERR(const void *ptr) { return (long) ptr; } int main(int argc, const char *argv[]) { void *ptr = (void *)-1; printf("ptr = %p, -PTR_ERR(ptr) = %ld\n\n", ptr, -PTR_ERR(ptr)); if(IS_ERR(ptr)) { if (-PTR_ERR(ptr) < 0) printf("That's what the codes want.\n"); else printf("Bug happens!\n"); } return 0; } ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] btusb: fix overflow return values 2013-07-09 2:55 ` Adam Lee 2013-07-09 7:40 ` Adam Lee @ 2013-07-09 15:32 ` Gustavo Padovan 2013-07-10 2:02 ` [PATCH v3] btusb: fix wrong use of PTR_ERR() Adam Lee 1 sibling, 1 reply; 11+ messages in thread From: Gustavo Padovan @ 2013-07-09 15:32 UTC (permalink / raw) To: Adam Lee Cc: Marcel Holtmann, linux-bluetooth, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Johan Hedberg, open list Hi Adam, * Adam Lee <adam.lee@canonical.com> [2013-07-09 10:55:01 +0800]: > On Mon, Jul 08, 2013 at 11:50:54AM -0700, Marcel Holtmann wrote: > > Hi Adam, > > > > > PTR_ERR() returns a long type value, but btusb_setup_intel() and > > > btusb_setup_intel_patching() should return an int type value. > > > > > > This bug makes the judgement "if (ret < 0)" not working on x86_64 > > > architecture systems, leading to failure as below, even panic. > > > ... > > > For not affecting other modules, I choose to modify the return values > > > but not extend btusb_setup_intel() and btusb_setup_intel_patching()'s > > > return types. This is harmless, because the return values were only > > > used to comparing number 0. > > > > there are tons of examples in various subsystems and drivers where we > > return PTR_ERR from a function calls returning int. > > > > So I wonder what is actually going wrong here. If this is x86_64 > > specific problem with PTR_ERR vs int, then we should have this problem > > everywhere in the kernel. > > Hi, Marcel > > I see you point, the difference between here and other subsystems are: > > 1, it returns -PTR_ERR() here but all other places return PTR_ERR(), I > checked. Please sending a patch fixing this. We got this right in other parts of the bluetooth subsystems but somehow we failed to check this when this code came in. And then another updating the checks if needed. Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3] btusb: fix wrong use of PTR_ERR() 2013-07-09 15:32 ` [PATCH] btusb: fix overflow return values Gustavo Padovan @ 2013-07-10 2:02 ` Adam Lee 2013-07-10 10:28 ` Gustavo Padovan 0 siblings, 1 reply; 11+ messages in thread From: Adam Lee @ 2013-07-10 2:02 UTC (permalink / raw) To: Gustavo Padovan Cc: linux-bluetooth, Marcel Holtmann, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Gustavo Padovan, Johan Hedberg, open list PTR_ERR() returns a signed long type value which is limited by IS_ERR(), it must be a negative number whose range is [-MAX_ERRNO, 0). The bug here returns negative numbers as error codes, then check it by "if (ret < 0)", but -PTR_ERR() is actually positive. The wrong use here leads to failure as below, even panic. [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110) [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110) [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110) [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110) [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000] Signed-off-by: Adam Lee <adam.lee@canonical.com> --- drivers/bluetooth/btusb.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 7a7e5f8..23df968 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -1092,7 +1092,7 @@ static int btusb_setup_intel_patching(struct hci_dev *hdev, if (IS_ERR(skb)) { BT_ERR("%s sending Intel patch command (0x%4.4x) failed (%ld)", hdev->name, cmd->opcode, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } /* It ensures that the returned event matches the event data read from @@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) if (IS_ERR(skb)) { BT_ERR("%s sending initial HCI reset command failed (%ld)", hdev->name, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } kfree_skb(skb); @@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) if (IS_ERR(skb)) { BT_ERR("%s reading Intel fw version command failed (%ld)", hdev->name, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } if (skb->len != sizeof(*ver)) { @@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) BT_ERR("%s entering Intel manufacturer mode failed (%ld)", hdev->name, PTR_ERR(skb)); release_firmware(fw); - return -PTR_ERR(skb); + return PTR_ERR(skb); } if (skb->data[0]) { @@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) if (IS_ERR(skb)) { BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", hdev->name, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } kfree_skb(skb); @@ -1289,7 +1289,7 @@ exit_mfg_disable: if (IS_ERR(skb)) { BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", hdev->name, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } kfree_skb(skb); @@ -1307,7 +1307,7 @@ exit_mfg_deactivate: if (IS_ERR(skb)) { BT_ERR("%s exiting Intel manufacturer mode failed (%ld)", hdev->name, PTR_ERR(skb)); - return -PTR_ERR(skb); + return PTR_ERR(skb); } kfree_skb(skb); -- 1.8.3.2 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3] btusb: fix wrong use of PTR_ERR() 2013-07-10 2:02 ` [PATCH v3] btusb: fix wrong use of PTR_ERR() Adam Lee @ 2013-07-10 10:28 ` Gustavo Padovan 0 siblings, 0 replies; 11+ messages in thread From: Gustavo Padovan @ 2013-07-10 10:28 UTC (permalink / raw) To: Adam Lee Cc: linux-bluetooth, Marcel Holtmann, Wen-chien Jesse Sung, AceLan Kao, Tedd Ho-Jeong An, Anthony Wong, Johan Hedberg, open list Hi Adam, * Adam Lee <adam8157@gmail.com> [2013-07-10 10:02:12 +0800]: > PTR_ERR() returns a signed long type value which is limited by IS_ERR(), > it must be a negative number whose range is [-MAX_ERRNO, 0). > > The bug here returns negative numbers as error codes, then check it by > "if (ret < 0)", but -PTR_ERR() is actually positive. The wrong use here > leads to failure as below, even panic. > > [ 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout > [ 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout > [ 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout > [ 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout > [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout > [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout > [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout > [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout > [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout > [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) failed (-110) > [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout > [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) failed (-110) > [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout > [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) failed (-110) > [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout > [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) failed (-110) > [ 112.907293] Bluetooth: hci0 command 0xfc11 tx timeout > [ 120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed (-110) > [ 120.904757] bluetoothd[1030]: segfault at 4 ip 00007f8b2eb55236 sp 00007fff53ff6920 error 4 in bluetoothd[7f8b2eaff000+cb000] > > Signed-off-by: Adam Lee <adam.lee@canonical.com> > --- > drivers/bluetooth/btusb.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) Patch has been applied to bluetooth.git. Thanks. Gustavo ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-07-10 10:28 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-07-04 12:43 [PATCH] btusb: fix overflow return values Adam Lee
[not found] ` <CAO_0yfOXfO1ZNTNbYRnuY-CQ44hH+DqoEB5jnCwfhLPTq-RUqQ@mail.gmail.com>
2013-07-05 2:53 ` Yang Bai
2013-07-05 2:59 ` Adam Lee
2013-07-05 4:41 ` Adam Lee
2013-07-08 18:50 ` Marcel Holtmann
2013-07-09 2:55 ` Adam Lee
2013-07-09 7:40 ` Adam Lee
2013-07-09 8:48 ` [PATCH v2] btusb: fix wrong use of PTR_ERR() Adam Lee
2013-07-09 15:32 ` [PATCH] btusb: fix overflow return values Gustavo Padovan
2013-07-10 2:02 ` [PATCH v3] btusb: fix wrong use of PTR_ERR() Adam Lee
2013-07-10 10:28 ` Gustavo Padovan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).