* [PATCH] android/scpp: Fix using freed memory @ 2014-12-22 11:49 Andrei Emeltchenko 2014-12-22 13:13 ` Szymon Janc 0 siblings, 1 reply; 4+ messages in thread From: Andrei Emeltchenko @ 2014-12-22 11:49 UTC (permalink / raw) To: linux-bluetooth From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> 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) -- 2.1.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] android/scpp: Fix using freed memory 2014-12-22 11:49 [PATCH] android/scpp: Fix using freed memory Andrei Emeltchenko @ 2014-12-22 13:13 ` Szymon Janc 2014-12-22 13:30 ` Andrei Emeltchenko 0 siblings, 1 reply; 4+ messages in thread From: Szymon Janc @ 2014-12-22 13:13 UTC (permalink / raw) To: Andrei Emeltchenko; +Cc: linux-bluetooth Hi Andrei, On Monday 22 of December 2014 13:49:13 Andrei Emeltchenko wrote: > From: Andrei Emeltchenko <andrei.emeltchenko@intel.com> > > 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); Although here scan and userdata seems reluctant (since req is already holding sccp instance). I'd wait for Luiz to comment on this since he wrote most of the scpp code. -- BR Szymon Janc ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] android/scpp: Fix using freed memory 2014-12-22 13:13 ` Szymon Janc @ 2014-12-22 13:30 ` Andrei Emeltchenko 2014-12-22 14:07 ` Luiz Augusto von Dentz 0 siblings, 1 reply; 4+ messages in thread From: Andrei Emeltchenko @ 2014-12-22 13:30 UTC (permalink / raw) To: Szymon Janc; +Cc: linux-bluetooth 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@intel.com> > > > > 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. Best regards Andrei Emeltchenko ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] android/scpp: Fix using freed memory 2014-12-22 13:30 ` Andrei Emeltchenko @ 2014-12-22 14:07 ` Luiz Augusto von Dentz 0 siblings, 0 replies; 4+ messages in thread From: Luiz Augusto von Dentz @ 2014-12-22 14:07 UTC (permalink / raw) To: Andrei Emeltchenko, Szymon Janc, linux-bluetooth@vger.kernel.org Hi Andrei, On Mon, Dec 22, 2014 at 11:30 AM, Andrei Emeltchenko <Andrei.Emeltchenko.news@gmail.com> 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@intel.com> >> > >> > 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-12-22 14:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-12-22 11:49 [PATCH] android/scpp: Fix using freed memory Andrei Emeltchenko 2014-12-22 13:13 ` Szymon Janc 2014-12-22 13:30 ` Andrei Emeltchenko 2014-12-22 14:07 ` Luiz Augusto von Dentz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox