Hi Andrei,
On Mon, Dec 22, 2014 at 11:30 AM, Andrei Emeltchenko
<Andrei.Emeltchenko.news@xxxxxxxxx> wrote:
> Hi Szymon,
>
> On Mon, Dec 22, 2014 at 02:13:53PM +0100, Szymon Janc wrote:
>> Hi Andrei,
>>
>> On Monday 22 of December 2014 13:49:13 Andrei Emeltchenko wrote:
>> > From: Andrei Emeltchenko <andrei.emeltchenko@xxxxxxxxx>
>> >
>> > Fixes use after free memory bug.
>> > req is assigned to user_data and then freed with destroy_gatt_req(req)
>> > ---
>> > android/scpp.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/android/scpp.c b/android/scpp.c
>> > index 77f48cd..9f60c9f 100644
>> > --- a/android/scpp.c
>> > +++ b/android/scpp.c
>> > @@ -301,8 +301,6 @@ static void refresh_discovered_cb(uint8_t status, GSList
>> > *chars, uint16_t start, end;
>> > bt_uuid_t uuid;
>> >
>> > - destroy_gatt_req(req);
>> > -
>> > if (status) {
>> > error("Scan Refresh %s", att_ecode2str(status));
>> > return;
>> > @@ -329,6 +327,8 @@ static void refresh_discovered_cb(uint8_t status, GSList
>> > *chars,
>> >
>> > discover_desc(scan, scan->attrib, start, end, &uuid,
>> > discover_descriptor_cb, user_data);
>> > +
>> > + destroy_gatt_req(req);
>> > }
>> >
>> > static void iwin_discovered_cb(uint8_t status, GSList *chars, void
>> > *user_data)
>>
>> This patch doesn't fix the bug (actually it introduces some memleaks).
>>
>> From what I see in code gatt_request is packing userdata passed to
>> discover_desc() so I think fix should be like:
>>
>> --- a/android/scpp.c
>> +++ b/android/scpp.c
>> @@ -328,7 +328,7 @@ static void refresh_discovered_cb(uint8_t status, GSList
>> *chars,
>> bt_uuid16_create(&uuid, GATT_CLIENT_CHARAC_CFG_UUID);
>>
>> discover_desc(scan, scan->attrib, start, end, &uuid,
>> - discover_descriptor_cb, user_data);
>> + discover_descriptor_cb, scan);
>
> This would access wrong memory since scan = req->user_data and req is
> freed as I mentioned in the patch above.
This would necessary cause a invalid access since destroy_gatt_req
only unref so only in case that actually frees the scan pointer this
would cause any problem but would mean that there is unbalance number
of reference. Now this code can be simplified because we do actually
have a queue holding the requests, in fact we only need the ids, so
there is no need to have request in the first place.
--
Luiz Augusto von Dentz
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
