2011-09-21 07:08:04

by Dan Carpenter

Subject: re: Bluetooth: Add secure flag for mgmt_pin_code_req

Hello Waldemar Rymarkiewicz,

This is a semi-automatic email about new static checker warnings.

Thu Apr 28 12:07:59 2011 +0200
a770bb5aea84: "Bluetooth: Add secure flag for mgmt_pin_code_req"

Leads to the following Smatch complaint:
net/bluetooth/hci_event.c +2189 hci_pin_code_request_evt()
error: we previously assumed 'conn' could be null (see line 2177)

net/bluetooth/hci_event.c
2176 conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
2177 if (conn && conn->state == BT_CONNECTED) {
^^^^
conn can be NULL.

2178 hci_conn_hold(conn);
2179 conn->disc_timeout = HCI_PAIRING_TIMEOUT;
2180 hci_conn_put(conn);
2181 }
2182
2183 if (!test_bit(HCI_PAIRABLE, &hdev->flags))
2184 hci_send_cmd(hdev, HCI_OP_PIN_CODE_NEG_REPLY,
2185 sizeof(ev->bdaddr), &ev->bdaddr);
2186 else if (test_bit(HCI_MGMT, &hdev->flags)) {
2187 u8 secure;
2188
2189 if (conn->pending_sec_level == BT_SECURITY_HIGH)
^^^^^^^^^^^^^^^^^^^^^^^
dereferenced unconditionally here.

2190 secure = 1;
2191 else

regards,
dan carpenter

2011-09-22 10:58:41

by Marcel Holtmann

Subject: Re: [PATCH] Bluetooth: Fix possible NULL pointer dereference

Hi Waldemar,

> >> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&ev->bdaddr);
> >> - if (conn&& conn->state == BT_CONNECTED) {
> >> - hci_conn_hold(conn);
> >> - conn->disc_timeout = HCI_PAIRING_TIMEOUT;
> >> - hci_conn_put(conn);
> >> - }
> >> + if (conn) {
> >
> > what is from with this:
> >
> > if (!conn)
> > goto unlock;
> >
>
> Well, the file is mixed in style, thus I chose the style as for nearby
> functions. Anyway, if this form is preferable let me know and will update.

for simple code pieces, I don't really mind, but this function is
getting more and more complicated. So less indentation is preferred.

Regards

Marcel

2011-09-22 10:28:15

by Rymarkiewicz Waldemar

Subject: Re: [PATCH] Bluetooth: Fix possible NULL pointer dereference

Marcel,

>> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK,&ev->bdaddr);
>> - if (conn&& conn->state == BT_CONNECTED) {
>> - hci_conn_hold(conn);
>> - conn->disc_timeout = HCI_PAIRING_TIMEOUT;
>> - hci_conn_put(conn);
>> - }
>> + if (conn) {
>
> what is from with this:
>
> if (!conn)
> goto unlock;
>

Well, the file is mixed in style, thus I chose the style as for nearby
functions. Anyway, if this form is preferable let me know and will update.

/Waldek

2011-09-22 09:51:40

by Marcel Holtmann

Subject: Re: [PATCH] Bluetooth: Fix possible NULL pointer dereference

Hi Waldemar,

> Checking conn->pending_sec_level if there is no connection leads to potential
> null pointer dereference. Don't process pin_code_request_event at all if no
> connection exists.
>
> Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
> ---
> net/bluetooth/hci_event.c | 30 ++++++++++++++++--------------
> 1 files changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index a520787..41c2562 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -2175,24 +2175,26 @@ static inline void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff
> hci_dev_lock(hdev);
>
> conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
> - if (conn && conn->state == BT_CONNECTED) {
> - hci_conn_hold(conn);
> - conn->disc_timeout = HCI_PAIRING_TIMEOUT;
> - hci_conn_put(conn);
> - }
> + if (conn) {

what is from with this:

if (!conn)
goto unlock;

Regards

Marcel

2011-09-22 05:58:12

by Rymarkiewicz Waldemar

Subject: [PATCH] Bluetooth: Fix possible NULL pointer dereference

From: Waldemar Rymarkiewicz <[email protected]>

Checking conn->pending_sec_level if there is no connection leads to potential
null pointer dereference. Don't process pin_code_request_event at all if no
connection exists.

Signed-off-by: Waldemar Rymarkiewicz <[email protected]>
---
net/bluetooth/hci_event.c | 30 ++++++++++++++++--------------
1 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index a520787..41c2562 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -2175,24 +2175,26 @@ static inline void hci_pin_code_request_evt(struct hci_dev *hdev, struct sk_buff
hci_dev_lock(hdev);

conn = hci_conn_hash_lookup_ba(hdev, ACL_LINK, &ev->bdaddr);
- if (conn && conn->state == BT_CONNECTED) {
- hci_conn_hold(conn);
- conn->disc_timeout = HCI_PAIRING_TIMEOUT;
- hci_conn_put(conn);
- }
+ if (conn) {
+ if (conn->state == BT_CONNECTED) {
+ hci_conn_hold(conn);
+ conn->disc_timeout = HCI_PAIRING_TIMEOUT;
+ hci_conn_put(conn);
+ }

- if (!test_bit(HCI_PAIRABLE, &hdev->flags))
- hci_send_cmd(hdev, HCI_OP_PIN_CODE_NEG_REPLY,
+ if (!test_bit(HCI_PAIRABLE, &hdev->flags))
+ hci_send_cmd(hdev, HCI_OP_PIN_CODE_NEG_REPLY,
sizeof(ev->bdaddr), &ev->bdaddr);
- else if (test_bit(HCI_MGMT, &hdev->flags)) {
- u8 secure;
+ else if (test_bit(HCI_MGMT, &hdev->flags)) {
+ u8 secure;

- if (conn->pending_sec_level == BT_SECURITY_HIGH)
- secure = 1;
- else
- secure = 0;
+ if (conn->pending_sec_level == BT_SECURITY_HIGH)
+ secure = 1;
+ else
+ secure = 0;

- mgmt_pin_code_request(hdev->id, &ev->bdaddr, secure);
+ mgmt_pin_code_request(hdev->id, &ev->bdaddr, secure);
+ }
}

hci_dev_unlock(hdev);
--
1.7.6.3

2011-09-21 13:17:32

by Rymarkiewicz Waldemar

Subject: Re: Bluetooth: Add secure flag for mgmt_pin_code_req

> dereferenced unconditionally here.
>
> 2190 secure = 1;
> 2191 else

Obviously, will sent a patch in a while.

Thanks,
/Waldek