re: Bluetooth: Add secure flag for mgmt_pin_code_req

re: Bluetooth: Add secure flag for mgmt_pin_code_req

[Dan Carpenter]

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

Re: Bluetooth: Add secure flag for mgmt_pin_code_req

[Rymarkiewicz Waldemar]

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

Obviously, will sent a patch in a while.

Thanks,
/Waldek

[PATCH] Bluetooth: Fix possible NULL pointer dereference

[Waldemar Rymarkiewicz]

From: Waldemar Rymarkiewicz <waldemar.rymarkiewicz@gmail.com>

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 <waldemar.rymarkiewicz@gmail.com>
---
 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

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

[Marcel Holtmann]

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 <waldemar.rymarkiewicz@gmail.com>
> ---
>  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

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

[Rymarkiewicz Waldemar]

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

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

[Marcel Holtmann]

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

[PATCH] Bluetooth : Fix possible NULL pointer dereference

[Thomas Gagneret]

If we disconnect a device before completing the connection, connection
will no longer be available in connection list, thus conn will be NULL.

Signed-off-by: Thomas Gagneret <tgagneret@witekio.com>
---
 net/bluetooth/hci_conn.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index dc59eae54717..901731c16136 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -723,20 +723,17 @@ static void create_le_conn_complete(struct hci_dev *hdev, u8 status, u16 opcode)
 	hci_dev_lock(hdev);
 
 	conn = hci_lookup_le_connect(hdev);
+	if (!conn)
+		goto done;
 
 	if (!status) {
 		hci_connect_le_scan_cleanup(conn);
-		goto done;
+	} else {
+		BT_ERR("HCI request failed to create LE connection: status 0x%2.2x",
+			status);
+		hci_le_conn_failed(conn, status);
 	}
 
-	BT_ERR("HCI request failed to create LE connection: status 0x%2.2x",
-	       status);
-
-	if (!conn)
-		goto done;
-
-	hci_le_conn_failed(conn, status);
-
 done:
 	hci_dev_unlock(hdev);
 }
-- 
2.13.1