[PATCH] bluetooth: Fix possible NULL dereference

[PATCH] bluetooth: Fix possible NULL dereference

[Andrei Emeltchenko]

From: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

conn might be NULL and would be dereferenced in conn_set_key()
---
 net/bluetooth/hci_event.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index bd0a801..95f8057 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -3312,7 +3312,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
 	/* Update connection information since adding the key will have
 	 * fixed up the type in the case of changed combination keys.
 	 */
-	if (ev->key_type == HCI_LK_CHANGED_COMBINATION)
+	if (conn && ev->key_type == HCI_LK_CHANGED_COMBINATION)
 		conn_set_key(conn, key->type, key->pin_len);
 
 	mgmt_new_link_key(hdev, key, persistent);
-- 
1.9.1

Re: [PATCH] bluetooth: Fix possible NULL dereference

[Marcel Holtmann]

Hi Andrei,

> conn might be NULL and would be dereferenced in conn_set_key()
> ---
> net/bluetooth/hci_event.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> index bd0a801..95f8057 100644
> --- a/net/bluetooth/hci_event.c
> +++ b/net/bluetooth/hci_event.c
> @@ -3312,7 +3312,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
> 	/* Update connection information since adding the key will have
> 	 * fixed up the type in the case of changed combination keys.
> 	 */
> -	if (ev->key_type == HCI_LK_CHANGED_COMBINATION)
> +	if (conn && ev->key_type == HCI_LK_CHANGED_COMBINATION)
> 		conn_set_key(conn, key->type, key->pin_len);
> 
> 	mgmt_new_link_key(hdev, key, persistent);

the more important question is why we are considering link keys from the controller for a device that we do not have a hci_conn object for.

If this is for security mode 3, then we should handle security mode 3 and then bail out of this function. Since security mode 3 means that we never get SSP or SC based keys. It is legacy pairing and does not need any special handling for debug keys or changed combination keys.

Regards

Marcel

Re: [PATCH] bluetooth: Fix possible NULL dereference

[Johan Hedberg]

Hi Marcel,

On Wed, Dec 03, 2014, Marcel Holtmann wrote:
> > conn might be NULL and would be dereferenced in conn_set_key()
> > ---
> > net/bluetooth/hci_event.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index bd0a801..95f8057 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -3312,7 +3312,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
> > 	/* Update connection information since adding the key will have
> > 	 * fixed up the type in the case of changed combination keys.
> > 	 */
> > -	if (ev->key_type == HCI_LK_CHANGED_COMBINATION)
> > +	if (conn && ev->key_type == HCI_LK_CHANGED_COMBINATION)
> > 		conn_set_key(conn, key->type, key->pin_len);
> > 
> > 	mgmt_new_link_key(hdev, key, persistent);
> 
> the more important question is why we are considering link keys from
> the controller for a device that we do not have a hci_conn object for.
> 
> If this is for security mode 3, then we should handle security mode 3
> and then bail out of this function. Since security mode 3 means that
> we never get SSP or SC based keys. It is legacy pairing and does not
> need any special handling for debug keys or changed combination keys.

The existence of a hci_conn object in this case is not dependent on
whether we're in security mode 3 or not. The hci_conn should have either
way been created when we initiated the connection or received a
connection request (only speciality with sec mode 3 is when the conn
complete finally comes). So this particular fix is really only for a
coverity warning as far as I see (and some very broken controllers).

As for security mode 3 connections we would still want to make a note of
the key type and PIN length.

Johan

Re: [PATCH] bluetooth: Fix possible NULL dereference

[Marcel Holtmann]

Hi Johan,

>>> conn might be NULL and would be dereferenced in conn_set_key()
>>> ---
>>> net/bluetooth/hci_event.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
>>> index bd0a801..95f8057 100644
>>> --- a/net/bluetooth/hci_event.c
>>> +++ b/net/bluetooth/hci_event.c
>>> @@ -3312,7 +3312,7 @@ static void hci_link_key_notify_evt(struct hci_dev *hdev, struct sk_buff *skb)
>>> 	/* Update connection information since adding the key will have
>>> 	 * fixed up the type in the case of changed combination keys.
>>> 	 */
>>> -	if (ev->key_type == HCI_LK_CHANGED_COMBINATION)
>>> +	if (conn && ev->key_type == HCI_LK_CHANGED_COMBINATION)
>>> 		conn_set_key(conn, key->type, key->pin_len);
>>> 
>>> 	mgmt_new_link_key(hdev, key, persistent);
>> 
>> the more important question is why we are considering link keys from
>> the controller for a device that we do not have a hci_conn object for.
>> 
>> If this is for security mode 3, then we should handle security mode 3
>> and then bail out of this function. Since security mode 3 means that
>> we never get SSP or SC based keys. It is legacy pairing and does not
>> need any special handling for debug keys or changed combination keys.
> 
> The existence of a hci_conn object in this case is not dependent on
> whether we're in security mode 3 or not. The hci_conn should have either
> way been created when we initiated the connection or received a
> connection request (only speciality with sec mode 3 is when the conn
> complete finally comes). So this particular fix is really only for a
> coverity warning as far as I see (and some very broken controllers).
> 
> As for security mode 3 connections we would still want to make a note of
> the key type and PIN length.

then please look through the whole function. We have if (conn) checks sprinkled all over it. This needs to be re-written to make it clean.

All the handling for SSP or SC should not even be considered when we do not have hci_conn object. I would say, do the minimum if we do not have hci_conn object and bail out. And let the SSP and SC pieces be executed otherwise.

Regards

Marcel