[PATCH BlueZ] gdbus: Fix crash error when calling g_dbus_remove_all_watches

[PATCH BlueZ] gdbus: Fix crash error when calling g_dbus_remove_all_watches

[Tomasz Bursztyka]

---
Hi,

While using gdbus on some other code, I found out that bug around g_dbus_remove_all_watches() usage.

Tomasz

 gdbus/watch.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index d749176..968a38a 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -298,6 +298,9 @@ static void filter_data_call_and_free(struct filter_data *data)
 		g_free(cb);
 	}
 
+	g_slist_free(data->callbacks);
+	data->callbacks = NULL;
+
 	filter_data_free(data);
 }
 
-- 
1.7.8.6

Re: [PATCH BlueZ] gdbus: Fix crash error when calling g_dbus_remove_all_watches

[Johan Hedberg]

Hi Thomasz,

On Fri, Aug 24, 2012, Tomasz Bursztyka wrote:
> ---
> Hi,
> 
> While using gdbus on some other code, I found out that bug around g_dbus_remove_all_watches() usage.
> 
> Tomasz
> 
>  gdbus/watch.c |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index d749176..968a38a 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -298,6 +298,9 @@ static void filter_data_call_and_free(struct filter_data *data)
>  		g_free(cb);
>  	}
>  
> +	g_slist_free(data->callbacks);
> +	data->callbacks = NULL;
> +
>  	filter_data_free(data);
>  }

It seems this patch never got applied. Is it so that no-one else has
seen the issue. Could someone (through basic static analysis) confirm if
the patch is correct? It'd be nice if we could also have a back trace of
the crash in the commit message.

Johan

Re: [PATCH BlueZ] gdbus: Fix crash error when calling g_dbus_remove_all_watches

[Tomasz Bursztyka]

Hi Johan,

this bug never hasn't hit anyone since the function 
g_dbus_remove_all_watches() is not used anywhere (but in my project)

I will resend the patch with the backtrace.

Tomasz
> It seems this patch never got applied. Is it so that no-one else has
> seen the issue. Could someone (through basic static analysis) confirm if
> the patch is correct? It'd be nice if we could also have a back trace of
> the crash in the commit message.
>
> Johan
>

[PATCH] gdbus: Fix double free when calling g_dbus_remove_all_watches

[Tomasz Bursztyka]

Valgrind output:
==21779== Invalid free() / delete / delete[] / realloc()
==21779==    at 0x4A080BC: free (vg_replace_malloc.c:427)
==21779==    by 0x406700: filter_data_free (watch.c:285)
==21779==    by 0x406D92: g_dbus_remove_all_watches (watch.c:315)
==21779==    by 0x408787: connman_interface_finalize (interface.c:99)
==21779==    by 0x40EA14: main (main.c:113)
==21779==  Address 0x6893810 is 0 bytes inside a block of size 56 free'd
==21779==    at 0x4A080BC: free (vg_replace_malloc.c:427)
==21779==    by 0x406D81: g_dbus_remove_all_watches (watch.c:309)
==21779==    by 0x408787: connman_interface_finalize (interface.c:99)
==21779==    by 0x40EA14: main (main.c:113)
---
Hi Johan,

I finally put the valgrind output, since backtrace is actually useless:

*** glibc detected *** ./project: double free or corruption (fasttop): 0x000000000075b5a0 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7adf5)[0x7f6701e5adf5]
./project[0x4066b1]
./project[0x406d43]
./project[0x408738]
./project[0x40e9a5]
/lib64/libc.so.6(__libc_start_main+0xfd)[0x7f6701e024bd]
./project[0x4053f9] 

Anyway, this bug was never found before for a good reason: no projects (but mine) uses g_dbus_remove_all_watches()

 gdbus/watch.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gdbus/watch.c b/gdbus/watch.c
index 9e4f994..9451d5d 100644
--- a/gdbus/watch.c
+++ b/gdbus/watch.c
@@ -309,6 +309,9 @@ static void filter_data_call_and_free(struct filter_data *data)
 		g_free(cb);
 	}
 
+	g_slist_free(data->callbacks);
+	data->callbacks = NULL;
+
 	filter_data_free(data);
 }
 
-- 
1.8.0.2

Re: [PATCH] gdbus: Fix double free when calling g_dbus_remove_all_watches

[Lucas De Marchi]

On Wed, Dec 19, 2012 at 10:01 AM, Tomasz Bursztyka
<tomasz.bursztyka@linux.intel.com> wrote:
> Valgrind output:
> ==21779== Invalid free() / delete / delete[] / realloc()
> ==21779==    at 0x4A080BC: free (vg_replace_malloc.c:427)
> ==21779==    by 0x406700: filter_data_free (watch.c:285)
> ==21779==    by 0x406D92: g_dbus_remove_all_watches (watch.c:315)
> ==21779==    by 0x408787: connman_interface_finalize (interface.c:99)
> ==21779==    by 0x40EA14: main (main.c:113)
> ==21779==  Address 0x6893810 is 0 bytes inside a block of size 56 free'd
> ==21779==    at 0x4A080BC: free (vg_replace_malloc.c:427)
> ==21779==    by 0x406D81: g_dbus_remove_all_watches (watch.c:309)
> ==21779==    by 0x408787: connman_interface_finalize (interface.c:99)
> ==21779==    by 0x40EA14: main (main.c:113)
> ---
> Hi Johan,
>
> I finally put the valgrind output, since backtrace is actually useless:
>
> *** glibc detected *** ./project: double free or corruption (fasttop): 0x000000000075b5a0 ***
> ======= Backtrace: =========
> /lib64/libc.so.6(+0x7adf5)[0x7f6701e5adf5]
> ./project[0x4066b1]
> ./project[0x406d43]
> ./project[0x408738]
> ./project[0x40e9a5]
> /lib64/libc.so.6(__libc_start_main+0xfd)[0x7f6701e024bd]
> ./project[0x4053f9]
>
> Anyway, this bug was never found before for a good reason: no projects (but mine) uses g_dbus_remove_all_watches()
>
>  gdbus/watch.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/gdbus/watch.c b/gdbus/watch.c
> index 9e4f994..9451d5d 100644
> --- a/gdbus/watch.c
> +++ b/gdbus/watch.c
> @@ -309,6 +309,9 @@ static void filter_data_call_and_free(struct filter_data *data)
>                 g_free(cb);
>         }
>
> +       g_slist_free(data->callbacks);
> +       data->callbacks = NULL;
> +


why not just removing the g_free(cb)  above?


Lucas De Marchi

Re: [PATCH] gdbus: Fix double free when calling g_dbus_remove_all_watches

[Tomasz Bursztyka]

Hi Lucas,

> @@ -309,6 +309,9 @@ static void filter_data_call_and_free(struct filter_data *data)
>                  g_free(cb);
>          }
>
> +       g_slist_free(data->callbacks);
> +       data->callbacks = NULL;
> +
>
> why not just removing the g_free(cb)  above?
>

I did so to avoid filter_data_free() to go through the list once again,
but indeed if the list is not going to be big, it's superfluous then.

Tomasz

Authentication issue during connection if remote deletes link key

[Jaganath Kanakkassery]

Hi,

I am facing one issue with authentication during profile connection.
Two devices are there A and B which are paired.
>From B I deleted the pairing of A.
Now from A I initiated a file transfer to B which failed during 
authentication
since remote link key is not there.
Please see the hcidump logs.
< HCI Command: Authentication Requested (0x01|0x0011) plen 2
    handle 12
< ACL data: handle 12 flags 0x00 dlen 12
    L2CAP(s): Disconn req: dcid 0x0046 scid 0x0040
> HCI Event: Command Status (0x0f) plen 4
    Authentication Requested (0x01|0x0011) status 0x00 ncmd 1
> HCI Event: Link Key Request (0x17) plen 6
    bdaddr BC:85:1F:74:7F:29
< HCI Command: Link Key Request Reply (0x01|0x000b) plen 22
    bdaddr BC:85:1F:74:7F:29 key 0E7B8C5C01CBF6CA7FF08050591C21BB
> HCI Event: Command Complete (0x0e) plen 10
    Link Key Request Reply (0x01|0x000b) ncmd 1
    status 0x00 bdaddr BC:85:1F:74:7F:29
> ACL data: handle 12 flags 0x02 dlen 12
    L2CAP(s): Disconn rsp: dcid 0x0046 scid 0x0040
> HCI Event: Auth Complete (0x06) plen 3
    status 0x06 handle 12
    Error: PIN or Key Missing

But the same scenario with legacy pairing it is working fine. Pin code 
request is coming
from the controller.
< HCI Command: Authentication Requested (0x01|0x0011) plen 2
    handle 11
< ACL data: handle 11 flags 0x00 dlen 12
    L2CAP(s): Disconn req: dcid 0x0040 scid 0x0040
> HCI Event: Command Status (0x0f) plen 4
    Authentication Requested (0x01|0x0011) status 0x00 ncmd 1
> HCI Event: Link Key Request (0x17) plen 6
    bdaddr 00:80:98:E7:34:07
< HCI Command: Link Key Request Reply (0x01|0x000b) plen 22
    bdaddr 00:80:98:E7:34:07 key 0E174763D381917A5AF973E22C49A3FE
> HCI Event: Command Complete (0x0e) plen 10
    Link Key Request Reply (0x01|0x000b) ncmd 1
    status 0x00 bdaddr 00:80:98:E7:34:07
> ACL data: handle 11 flags 0x02 dlen 12
    L2CAP(s): Disconn rsp: dcid 0x0040 scid 0x0040
> HCI Event: PIN Code Request (0x16) plen 6
    bdaddr 00:80:98:E7:34:07
> HCI Event: Number of Completed Packets (0x13) plen 5
    handle 11 packets 1
< HCI Command: PIN Code Request Reply (0x01|0x000d) plen 23
    bdaddr 00:80:98:E7:34:07 len 1 pin '0'
> HCI Event: Command Complete (0x0e) plen 10
    PIN Code Request Reply (0x01|0x000d) ncmd 1
    status 0x00 bdaddr 00:80:98:E7:34:07
> HCI Event: Link Key Notification (0x18) plen 23
    bdaddr 00:80:98:E7:34:07 key 573CC496BB0C02EE461B4A18C2D758C0 type 0
    Type: Combination Key
> HCI Event: Auth Complete (0x06) plen 3
    status 0x00 handle 11


Is it controller issue or BlueZ has to take care this?
Please let me know your opinion.

Thanks,
Jaganath
 

Re: Authentication issue during connection if remote deletes link key

[Johan Hedberg]

Hi Jaganath,

On Thu, Dec 20, 2012, Jaganath Kanakkassery wrote:
> I am facing one issue with authentication during profile connection.
> Two devices are there A and B which are paired.
> From B I deleted the pairing of A.
> Now from A I initiated a file transfer to B which failed during
> authentication
> since remote link key is not there.
> Please see the hcidump logs.
> < HCI Command: Authentication Requested (0x01|0x0011) plen 2
>    handle 12
> < ACL data: handle 12 flags 0x00 dlen 12
>    L2CAP(s): Disconn req: dcid 0x0046 scid 0x0040
> >HCI Event: Command Status (0x0f) plen 4
>    Authentication Requested (0x01|0x0011) status 0x00 ncmd 1
> >HCI Event: Link Key Request (0x17) plen 6
>    bdaddr BC:85:1F:74:7F:29
> < HCI Command: Link Key Request Reply (0x01|0x000b) plen 22
>    bdaddr BC:85:1F:74:7F:29 key 0E7B8C5C01CBF6CA7FF08050591C21BB
> >HCI Event: Command Complete (0x0e) plen 10
>    Link Key Request Reply (0x01|0x000b) ncmd 1
>    status 0x00 bdaddr BC:85:1F:74:7F:29
> >ACL data: handle 12 flags 0x02 dlen 12
>    L2CAP(s): Disconn rsp: dcid 0x0046 scid 0x0040
> >HCI Event: Auth Complete (0x06) plen 3
>    status 0x06 handle 12
>    Error: PIN or Key Missing
> 
> Is it controller issue or BlueZ has to take care this?
> Please let me know your opinion.

This is normal behavior. The core spec gives us two options, either fail
or ask the user if he wants to try to repair. Right now the former is
implemented and you'll need to explicitly remove the local pairing
before attempting to repair.

We could (and maybe should) implement the latter option, but we need to
be very careful since it's important that we do not drop the old link
key until the new pairing is successful and we should also refuse to
accept the new pairing if it results in a less secure link key
(unauthenticated vs authenticated) than the original one. This is to
avoid attacks from devices spoofing the address of the other (paired)
device and therefore not having the link key available.

The way this should be implemented is that upon getting the "PIN or Key
Missing" error after having responded with a positive link key reply on
our side the kernel should fire off a mgmt event to user space to
request repairing (on the D-Bus agent level I think we could probably
reuse the RequestAuthorization callback for this). If a positive
response is received from user space the kernel would proceed with a new
pairing attempt, holding on to the old link key but still giving a
negative link key reply, and then follow the procedure which I described
in the previous paragraph.

If you're interested to implement this I'd be happy to provide more
feedback and guidance. The implementation should also be extended to LE
SMP where I believe we have a similar improvement opportunity.

Johan