[PATCH BlueZ] audio/AVCTP: Fix crash

[PATCH BlueZ] audio/AVCTP: Fix crash

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

The following crash happens because the list l is modified within the
loop so it is no longer safe to call l->next directly:

Invalid read of size 8
   at 0x41F276: pending_create (avctp.c:1491)
   by 0x41F7C0: avctp_send_req.isra.6 (avctp.c:1539)
   by 0x41F887: avctp_passthrough_release (avctp.c:1643)
   by 0x41F9DF: avctp_passthrough_rsp (avctp.c:1698)
   by 0x41E9AC: session_cb (avctp.c:782)
   by 0x31D1647DF5: g_main_context_dispatch (in /usr/lib64/libglib-2.0.so.0.3600.3)
   by 0x31D1648147: ??? (in /usr/lib64/libglib-2.0.so.0.3600.3)
   by 0x31D1648549: g_main_loop_run (in /usr/lib64/libglib-2.0.so.0.3600.3)
   by 0x40A49F: main (main.c:587)
 Address 0x8 is not stack'd, malloc'd or (recently) free'd
---
 profiles/audio/avctp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/profiles/audio/avctp.c b/profiles/audio/avctp.c
index 4de981c..6669ddc 100644
--- a/profiles/audio/avctp.c
+++ b/profiles/audio/avctp.c
@@ -1488,7 +1488,7 @@ static struct avctp_pending_req *pending_create(struct avctp_channel *chan,
 	tmp = g_slist_copy(chan->processed);
 
 	/* Find first unused transaction id */
-	for (l = tmp; l; l = l->next) {
+	for (l = tmp; l; l = g_slist_next(l)) {
 		struct avctp_pending_req *req = l->data;
 
 		if (req->transaction == chan->transaction) {
-- 
1.8.3.1

Re: [PATCH BlueZ] audio/AVCTP: Fix crash

[Anderson Lizardo]

Hi Luiz,

On Tue, Dec 3, 2013 at 5:36 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> --- a/profiles/audio/avctp.c
> +++ b/profiles/audio/avctp.c
> @@ -1488,7 +1488,7 @@ static struct avctp_pending_req *pending_create(struct avctp_channel *chan,
>         tmp = g_slist_copy(chan->processed);
>
>         /* Find first unused transaction id */
> -       for (l = tmp; l; l = l->next) {
> +       for (l = tmp; l; l = g_slist_next(l)) {

Are you sure this fixes the problem? AFAIK g_list_next() will still
access invalid memory unless the "next" pointer is saved *before* the
current entry is freed. See e.g. remove_temp_devices() in
src/adapter.c.

>                 struct avctp_pending_req *req = l->data;
>
>                 if (req->transaction == chan->transaction) {

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

Re: [PATCH BlueZ] audio/AVCTP: Fix crash

[Luiz Augusto von Dentz]

Hi Anderson,

On Tue, Dec 3, 2013 at 1:24 PM, Anderson Lizardo
<anderson.lizardo@openbossa.org> wrote:
> Hi Luiz,
>
> On Tue, Dec 3, 2013 at 5:36 AM, Luiz Augusto von Dentz
> <luiz.dentz@gmail.com> wrote:
>> --- a/profiles/audio/avctp.c
>> +++ b/profiles/audio/avctp.c
>> @@ -1488,7 +1488,7 @@ static struct avctp_pending_req *pending_create(struct avctp_channel *chan,
>>         tmp = g_slist_copy(chan->processed);
>>
>>         /* Find first unused transaction id */
>> -       for (l = tmp; l; l = l->next) {
>> +       for (l = tmp; l; l = g_slist_next(l)) {
>
> Are you sure this fixes the problem? AFAIK g_list_next() will still
> access invalid memory unless the "next" pointer is saved *before* the
> current entry is freed. See e.g. remove_temp_devices() in
> src/adapter.c.

Yep, I tested and it fixes the problem. The problem is not actually
removing the item, but reassigning tmp to the head of the list which
can be NULL so doing l = l->next before deleting the node doesn't help
in this case. Anyway this is now applied but we will be changing the
logic how to check the available transaction to use bitmask operations
so we find the next transaction without looping in a list.

Re: [PATCH BlueZ] audio/AVCTP: Fix crash

[Anderson Lizardo]

Hi Luiz,

On Tue, Dec 3, 2013 at 7:40 AM, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
> Hi Anderson,
>
> On Tue, Dec 3, 2013 at 1:24 PM, Anderson Lizardo
> <anderson.lizardo@openbossa.org> wrote:
>> Hi Luiz,
>>
>> On Tue, Dec 3, 2013 at 5:36 AM, Luiz Augusto von Dentz
>> <luiz.dentz@gmail.com> wrote:
>>> --- a/profiles/audio/avctp.c
>>> +++ b/profiles/audio/avctp.c
>>> @@ -1488,7 +1488,7 @@ static struct avctp_pending_req *pending_create(struct avctp_channel *chan,
>>>         tmp = g_slist_copy(chan->processed);
>>>
>>>         /* Find first unused transaction id */
>>> -       for (l = tmp; l; l = l->next) {
>>> +       for (l = tmp; l; l = g_slist_next(l)) {
>>
>> Are you sure this fixes the problem? AFAIK g_list_next() will still
>> access invalid memory unless the "next" pointer is saved *before* the
>> current entry is freed. See e.g. remove_temp_devices() in
>> src/adapter.c.
>
> Yep, I tested and it fixes the problem. The problem is not actually
> removing the item, but reassigning tmp to the head of the list which
> can be NULL so doing l = l->next before deleting the node doesn't help
> in this case. Anyway this is now applied but we will be changing the
> logic how to check the available transaction to use bitmask operations
> so we find the next transaction without looping in a list.

In this case I agree that it is not the same situation as I described.
I was looking only at the patch itself, not the original code :/ In
any case, good to know you are going to simplify it.

BTW, I noticed that the "chan->transaction %= 16;"  in that same
function (in two places) is unnecessary, because chan->transaction is
"uint8_t transaction:4" (4-bit bitfield) so the post increment
operation before this statement will always overflow "transaction" and
it will go back to 0 once it reaches 15.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil