[PATCH] btusb: fix overflow return values

[PATCH] btusb: fix overflow return values

[Adam Lee]

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

Re: [PATCH] btusb: fix overflow return values

[Yang Bai]

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

Re: [PATCH] btusb: fix overflow return values

[Adam Lee]

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

Re: [PATCH] btusb: fix overflow return values

[Adam Lee]

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

Re: [PATCH] btusb: fix overflow return values

[Marcel Holtmann]

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

Re: [PATCH] btusb: fix overflow return values

[Adam Lee]

[-- 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;
}

Re: [PATCH] btusb: fix overflow return values

[Adam Lee]

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

[PATCH v2] btusb: fix wrong use of PTR_ERR()

[Adam Lee]

[-- 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;
}

Re: [PATCH] btusb: fix overflow return values

[Gustavo Padovan]

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

[PATCH v3] btusb: fix wrong use of PTR_ERR()

[Adam Lee]

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

Re: [PATCH v3] btusb: fix wrong use of PTR_ERR()

[Gustavo Padovan]

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