Return-Path: MIME-Version: 1.0 In-Reply-To: <1372941783-30657-1-git-send-email-adam.lee@canonical.com> References: <1372941783-30657-1-git-send-email-adam.lee@canonical.com> From: Yang Bai Date: Fri, 5 Jul 2013 10:37:07 +0800 Message-ID: Subject: Re: [PATCH] btusb: fix overflow return values To: Adam Lee Cc: linux-bluetooth@vger.kernel.org, Wen-chien Jesse Sung , AceLan Kao , Tedd Ho-Jeong An , "Anthony Wong'" , Marcel Holtmann , Gustavo Padovan , Johan Hedberg , open list Content-Type: multipart/alternative; boundary=001a11c20b065e34d304e0ba93e8 List-ID: --001a11c20b065e34d304e0ba93e8 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable 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 =3D 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 =3D hdev->setup(hdev); if (!ret) { Thanks, Yang On Thu, Jul 4, 2013 at 8:43 PM, Adam Lee 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) faile= d > (-110) > [ 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout > [ 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout > [ 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout > [ 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout > [ 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout > [ 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout > [ 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) faile= d > (-110) > [ 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout > [ 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) faile= d > (-110) > [ 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout > [ 100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) faile= d > (-110) > [ 102.912858] Bluetooth: hci0 command 0xfc60 tx timeout > [ 110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) faile= d > (-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 > --- > 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_de= v > *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 !=3D 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" i= n > 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/ > --=20 """ Keep It Simple,Stupid. """ Chinese Name: =E7=99=BD=E6=9D=A8 Nick Name: Hamo Homepage: http://hamobai.com/ GPG KEY ID: 0xA4691A33 Key fingerprint =3D 09D5 2D78 8E2B 0995 CF8E 4331 33C4 3D24 A469 1A33 --001a11c20b065e34d304e0ba93e8 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
The return value of=C2=A0btusb_setup_intel is compared with 0. Code as:

<= /span>
drivers/bluetooth/btusb.c:
=
static int btusb_probe(str= uct usb_interface *intf,
const struct usb_device_id *id)
if (id->driver_info= & BTUSB_INTEL)
hdev->setup =3D btus= b_setup_intel;

ne= t/bluetooth/hci_core.c:
int hci_dev_open(__u16 dev)
if (hdev->setup && test_bit(HCI_SET= UP, &hdev->dev_flags))
ret =3D 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 btu= sb_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.

[ =C2=A0 12.958920] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 14.961765] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 16.964688] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 20.954501] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 22.957358] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 30.948922] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 32.951780] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 40.943359] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 42.946219] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 50.937812] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 52.940670] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 60.932236] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 62.935092] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 70.926688] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 72.929545] Bluetooth: hci0 command 0xfc8e tx timeout
[ =C2=A0 80.921111] Bluetooth: hci0 sending Intel patch command (0xfc8e) fa= iled (-110)
[ =C2=A0 82.923969] Bluetooth: hci0 command 0xfc2f tx timeout
[ =C2=A0 90.915542] Bluetooth: hci0 sending Intel patch command (0xfc2f) fa= iled (-110)
[ =C2=A0 92.918406] Bluetooth: hci0 command 0xfc11 tx timeout
[ =C2=A0100.909955] Bluetooth: hci0 sending Intel patch command (0xfc11) fa= iled (-110)
[ =C2=A0102.912858] Bluetooth: hci0 command 0xfc60 tx timeout
[ =C2=A0110.904394] Bluetooth: hci0 sending Intel patch command (0xfc60) fa= iled (-110)
[ =C2=A0112.907293] Bluetooth: hci0 command 0xfc11 tx timeout
[ =C2=A0120.898831] Bluetooth: hci0 exiting Intel manufacturer mode failed = (-110)
[ =C2=A0120.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>
---
=C2=A0drivers/bluetooth/btusb.c | 14 +++++++-------
=C2=A01 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,
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s sen= ding Intel patch command (0x%4.4x) failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, cmd->opcode, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* It ensures that the returned event matches t= he event data read from
@@ -1144,7 +1144,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s sen= ding initial HCI reset command failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);

@@ -1158,7 +1158,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s rea= ding Intel fw version command failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (skb->len !=3D sizeof(*ver)) {
@@ -1216,7 +1216,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s ent= ering Intel manufacturer mode failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 release_firmware(fw= );
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }

=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (skb->data[0]) {
@@ -1273,7 +1273,7 @@ static int btusb_setup_intel(struct hci_dev *hdev) =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s exi= ting Intel manufacturer mode failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);

@@ -1289,7 +1289,7 @@ exit_mfg_disable:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s exi= ting Intel manufacturer mode failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 kfree_skb(skb);

@@ -1307,7 +1307,7 @@ exit_mfg_deactivate:
=C2=A0 =C2=A0 =C2=A0 =C2=A0 if (IS_ERR(skb)) {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 BT_ERR("%s exi= ting Intel manufacturer mode failed (%ld)",
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0hdev->name, PTR_ERR(skb));
- =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -PTR_ERR(skb); + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return -EFAULT;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 }
=C2=A0 =C2=A0 =C2=A0 =C2=A0 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 major= domo@vger.kernel.org
More majordomo info at =C2=A0http://vger.kernel.org/majordomo-info.html Please read the FAQ at =C2=A0http://www.tux.org/lkml/



-- =C2=A0 =C2=A0 """
=C2=A0 =C2=A0 Keep It Simple,Stupid.= =C2=A0
=C2=A0 =C2=A0 """

Chinese Name: =E7=99=BD= =E6=9D=A8
Nick Name: Hamo
Homepage: h= ttp://hamobai.com/
GPG KEY ID: 0xA4691A33
Key fingerprint =3D 09D5 2D78 8E2B 0995 CF8E=C2= =A0 4331 33C4 3D24 A469 1A33
--001a11c20b065e34d304e0ba93e8--