From: Andrei Emeltchenko <[email protected]>
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
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
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
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