* [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout
@ 2021-10-08 19:00 Bernie Conrad
2021-10-08 19:24 ` Luiz Augusto von Dentz
2021-10-08 19:32 ` [BlueZ,v2] " bluez.test.bot
0 siblings, 2 replies; 4+ messages in thread
From: Bernie Conrad @ 2021-10-08 19:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bernie Conrad
From: Bernie Conrad <bernie@allthenticate.com>
The changes in gatt-database.c fix a use after free that was introduced
after the last cleanup patch, ccc_new and write_new operations were not
being properly unregistered because they were not assigned a disconn_id.
The changes in gatt-db add similar cleanup to pending reads/writes where
timeouts after a disconnect would cause a similar use after free with
already cleaned up resoureces, this adds a simple cb to set on a pending
read/write if a disconnect has occurred to skip the use.
v2: Fixing formatting issues
---
src/gatt-database.c | 4 ++--
src/shared/gatt-db.c | 38 ++++++++++++++++++++++++++++++++++++--
2 files changed, 38 insertions(+), 4 deletions(-)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index 475e7873c..00647cf08 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -978,7 +978,7 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
op->attrib = attrib;
op->link_type = link_type;
- bt_att_register_disconnect(att,
+ op->disconn_id = bt_att_register_disconnect(att,
pending_disconnect_cb,
op,
NULL);
@@ -2418,7 +2418,7 @@ static struct pending_op *pending_write_new(struct bt_att *att,
op->prep_authorize = prep_authorize;
queue_push_tail(owner_queue, op);
- bt_att_register_disconnect(att,
+ op->disconn_id = bt_att_register_disconnect(att,
pending_disconnect_cb,
op, NULL);
diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 3a02289ce..8423961f8 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c
@@ -77,17 +77,23 @@ struct attribute_notify {
struct pending_read {
struct gatt_db_attribute *attrib;
+ struct bt_att *att;
unsigned int id;
unsigned int timeout_id;
gatt_db_attribute_read_t func;
+ bool disconn;
+ unsigned int disconn_id;
void *user_data;
};
struct pending_write {
struct gatt_db_attribute *attrib;
+ struct bt_att *att;
unsigned int id;
unsigned int timeout_id;
gatt_db_attribute_write_t func;
+ bool disconn;
+ unsigned int disconn_id;
void *user_data;
};
@@ -139,8 +145,10 @@ static void pending_read_result(struct pending_read *p, int err,
if (p->timeout_id > 0)
timeout_remove(p->timeout_id);
- p->func(p->attrib, err, data, length, p->user_data);
+ if (!p->disconn)
+ p->func(p->attrib, err, data, length, p->user_data);
+ bt_att_unregister_disconnect(p->att, p->disconn_id);
free(p);
}
@@ -156,8 +164,10 @@ static void pending_write_result(struct pending_write *p, int err)
if (p->timeout_id > 0)
timeout_remove(p->timeout_id);
- p->func(p->attrib, err, p->user_data);
+ if (!p->disconn)
+ p->func(p->attrib, err, p->user_data);
+ bt_att_unregister_disconnect(p->att, p->disconn_id);
free(p);
}
@@ -1868,6 +1878,13 @@ bool gatt_db_attribute_set_fixed_length(struct gatt_db_attribute *attrib,
return true;
}
+static void pending_read_cb(int err, void *user_data)
+{
+ struct pending_read *p = user_data;
+
+ p->disconn = 1;
+}
+
bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
uint8_t opcode, struct bt_att *att,
gatt_db_attribute_read_t func, void *user_data)
@@ -1901,6 +1918,11 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
p->func = func;
p->user_data = user_data;
+ p->disconn = 0;
+ p->disconn_id = bt_att_register_disconnect(att,
+ pending_read_cb, p, NULL);
+ p->att = att;
+
queue_push_tail(attrib->pending_reads, p);
attrib->read_func(attrib, p->id, offset, opcode, att,
@@ -1956,6 +1978,13 @@ static bool write_timeout(void *user_data)
return false;
}
+static void pending_write_cb(int err, void *user_data)
+{
+ struct pending_write *p = user_data;
+
+ p->disconn = 1;
+}
+
bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
const uint8_t *value, size_t len,
uint8_t opcode, struct bt_att *att,
@@ -1995,6 +2024,11 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
p->func = func;
p->user_data = user_data;
+ p->disconn = 0;
+ p->disconn_id = bt_att_register_disconnect(att,
+ pending_write_cb, p, NULL);
+ p->att = att;
+
queue_push_tail(attrib->pending_writes, p);
attrib->write_func(attrib, p->id, offset, value, len, opcode,
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout
2021-10-08 19:00 [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout Bernie Conrad
@ 2021-10-08 19:24 ` Luiz Augusto von Dentz
2021-10-13 21:38 ` Luiz Augusto von Dentz
2021-10-08 19:32 ` [BlueZ,v2] " bluez.test.bot
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-10-08 19:24 UTC (permalink / raw)
To: Bernie Conrad; +Cc: linux-bluetooth, Bernie Conrad
Hi Bernie,
On Fri, Oct 8, 2021 at 12:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
>
> From: Bernie Conrad <bernie@allthenticate.com>
>
> The changes in gatt-database.c fix a use after free that was introduced
> after the last cleanup patch, ccc_new and write_new operations were not
> being properly unregistered because they were not assigned a disconn_id.
>
> The changes in gatt-db add similar cleanup to pending reads/writes where
> timeouts after a disconnect would cause a similar use after free with
> already cleaned up resoureces, this adds a simple cb to set on a pending
> read/write if a disconnect has occurred to skip the use.
>
> v2: Fixing formatting issues
>
> ---
> src/gatt-database.c | 4 ++--
> src/shared/gatt-db.c | 38 ++++++++++++++++++++++++++++++++++++--
> 2 files changed, 38 insertions(+), 4 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 475e7873c..00647cf08 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -978,7 +978,7 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
> op->attrib = attrib;
> op->link_type = link_type;
>
> - bt_att_register_disconnect(att,
> + op->disconn_id = bt_att_register_disconnect(att,
> pending_disconnect_cb,
> op,
> NULL);
> @@ -2418,7 +2418,7 @@ static struct pending_op *pending_write_new(struct bt_att *att,
> op->prep_authorize = prep_authorize;
> queue_push_tail(owner_queue, op);
>
> - bt_att_register_disconnect(att,
> + op->disconn_id = bt_att_register_disconnect(att,
> pending_disconnect_cb,
> op, NULL);
These changes above shall be split into another patch.
> diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> index 3a02289ce..8423961f8 100644
> --- a/src/shared/gatt-db.c
> +++ b/src/shared/gatt-db.c
> @@ -77,17 +77,23 @@ struct attribute_notify {
>
> struct pending_read {
> struct gatt_db_attribute *attrib;
> + struct bt_att *att;
> unsigned int id;
> unsigned int timeout_id;
> gatt_db_attribute_read_t func;
> + bool disconn;
> + unsigned int disconn_id;
> void *user_data;
> };
>
> struct pending_write {
> struct gatt_db_attribute *attrib;
> + struct bt_att *att;
> unsigned int id;
> unsigned int timeout_id;
> gatt_db_attribute_write_t func;
> + bool disconn;
> + unsigned int disconn_id;
> void *user_data;
> };
>
> @@ -139,8 +145,10 @@ static void pending_read_result(struct pending_read *p, int err,
> if (p->timeout_id > 0)
> timeout_remove(p->timeout_id);
>
> - p->func(p->attrib, err, data, length, p->user_data);
> + if (!p->disconn)
> + p->func(p->attrib, err, data, length, p->user_data);
>
> + bt_att_unregister_disconnect(p->att, p->disconn_id);
> free(p);
> }
>
> @@ -156,8 +164,10 @@ static void pending_write_result(struct pending_write *p, int err)
> if (p->timeout_id > 0)
> timeout_remove(p->timeout_id);
>
> - p->func(p->attrib, err, p->user_data);
> + if (!p->disconn)
> + p->func(p->attrib, err, p->user_data);
>
> + bt_att_unregister_disconnect(p->att, p->disconn_id);
> free(p);
> }
I wonder if it wouldn't be better to use a specific error to inform it
the operation has been aborted e.g. -ECONNABORTED instead of
duplicating the handling of disconnection, btw if we don't call the
callback who is doing the cleanup in gatt-server.c, we still need to
call async_read_op_destroy/async_write_op_destroy or that is taken
care somewhere else?
Also it would be great if we had a test in unit/test-gatt.c that
covers such scenarios, e.g disconnect while read/write is pending.
> @@ -1868,6 +1878,13 @@ bool gatt_db_attribute_set_fixed_length(struct gatt_db_attribute *attrib,
> return true;
> }
>
> +static void pending_read_cb(int err, void *user_data)
> +{
> + struct pending_read *p = user_data;
> +
> + p->disconn = 1;
> +}
> +
> bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> uint8_t opcode, struct bt_att *att,
> gatt_db_attribute_read_t func, void *user_data)
> @@ -1901,6 +1918,11 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> p->func = func;
> p->user_data = user_data;
>
> + p->disconn = 0;
> + p->disconn_id = bt_att_register_disconnect(att,
> + pending_read_cb, p, NULL);
> + p->att = att;
> +
> queue_push_tail(attrib->pending_reads, p);
>
> attrib->read_func(attrib, p->id, offset, opcode, att,
> @@ -1956,6 +1978,13 @@ static bool write_timeout(void *user_data)
> return false;
> }
>
> +static void pending_write_cb(int err, void *user_data)
> +{
> + struct pending_write *p = user_data;
> +
> + p->disconn = 1;
> +}
> +
> bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> const uint8_t *value, size_t len,
> uint8_t opcode, struct bt_att *att,
> @@ -1995,6 +2024,11 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> p->func = func;
> p->user_data = user_data;
>
> + p->disconn = 0;
> + p->disconn_id = bt_att_register_disconnect(att,
> + pending_write_cb, p, NULL);
> + p->att = att;
> +
> queue_push_tail(attrib->pending_writes, p);
>
> attrib->write_func(attrib, p->id, offset, value, len, opcode,
> --
> 2.17.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [BlueZ,v2] gatt: more fixes with cleanup on disconnect/timeout
2021-10-08 19:00 [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout Bernie Conrad
2021-10-08 19:24 ` Luiz Augusto von Dentz
@ 2021-10-08 19:32 ` bluez.test.bot
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2021-10-08 19:32 UTC (permalink / raw)
To: linux-bluetooth, bernie
[-- Attachment #1: Type: text/plain, Size: 2061 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=560135
---Test result---
Test Summary:
CheckPatch FAIL 0.53 seconds
GitLint PASS 0.32 seconds
Prep - Setup ELL PASS 49.68 seconds
Build - Prep PASS 0.19 seconds
Build - Configure PASS 8.87 seconds
Build - Make PASS 214.13 seconds
Make Check PASS 9.88 seconds
Make Distcheck PASS 253.34 seconds
Build w/ext ELL - Configure PASS 9.03 seconds
Build w/ext ELL - Make PASS 202.66 seconds
Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script with rule in .checkpatch.conf
Output:
[BlueZ,v2] gatt: more fixes with cleanup on disconnect/timeout
ERROR:CODE_INDENT: code indent should use tabs where possible
#191: FILE: src/shared/gatt-db.c:1923:
+^I^I^I^I ^Ipending_read_cb, p, NULL);$
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#191: FILE: src/shared/gatt-db.c:1923:
+^I^I^I^I ^Ipending_read_cb, p, NULL);$
/github/workspace/src/12546199.patch total: 1 errors, 1 warnings, 109 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
NOTE: Whitespace errors detected.
You may wish to use scripts/cleanpatch or scripts/cleanfile
/github/workspace/src/12546199.patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout
2021-10-08 19:24 ` Luiz Augusto von Dentz
@ 2021-10-13 21:38 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-10-13 21:38 UTC (permalink / raw)
To: Bernie Conrad; +Cc: linux-bluetooth, Bernie Conrad
Hi Bernie,
On Fri, Oct 8, 2021 at 12:24 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Bernie,
>
> On Fri, Oct 8, 2021 at 12:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
> >
> > From: Bernie Conrad <bernie@allthenticate.com>
> >
> > The changes in gatt-database.c fix a use after free that was introduced
> > after the last cleanup patch, ccc_new and write_new operations were not
> > being properly unregistered because they were not assigned a disconn_id.
> >
> > The changes in gatt-db add similar cleanup to pending reads/writes where
> > timeouts after a disconnect would cause a similar use after free with
> > already cleaned up resoureces, this adds a simple cb to set on a pending
> > read/write if a disconnect has occurred to skip the use.
> >
> > v2: Fixing formatting issues
> >
> > ---
> > src/gatt-database.c | 4 ++--
> > src/shared/gatt-db.c | 38 ++++++++++++++++++++++++++++++++++++--
> > 2 files changed, 38 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 475e7873c..00647cf08 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -978,7 +978,7 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
> > op->attrib = attrib;
> > op->link_type = link_type;
> >
> > - bt_att_register_disconnect(att,
> > + op->disconn_id = bt_att_register_disconnect(att,
> > pending_disconnect_cb,
> > op,
> > NULL);
> > @@ -2418,7 +2418,7 @@ static struct pending_op *pending_write_new(struct bt_att *att,
> > op->prep_authorize = prep_authorize;
> > queue_push_tail(owner_queue, op);
> >
> > - bt_att_register_disconnect(att,
> > + op->disconn_id = bt_att_register_disconnect(att,
> > pending_disconnect_cb,
> > op, NULL);
>
> These changes above shall be split into another patch.
>
> > diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
> > index 3a02289ce..8423961f8 100644
> > --- a/src/shared/gatt-db.c
> > +++ b/src/shared/gatt-db.c
> > @@ -77,17 +77,23 @@ struct attribute_notify {
> >
> > struct pending_read {
> > struct gatt_db_attribute *attrib;
> > + struct bt_att *att;
> > unsigned int id;
> > unsigned int timeout_id;
> > gatt_db_attribute_read_t func;
> > + bool disconn;
> > + unsigned int disconn_id;
> > void *user_data;
> > };
> >
> > struct pending_write {
> > struct gatt_db_attribute *attrib;
> > + struct bt_att *att;
> > unsigned int id;
> > unsigned int timeout_id;
> > gatt_db_attribute_write_t func;
> > + bool disconn;
> > + unsigned int disconn_id;
> > void *user_data;
> > };
> >
> > @@ -139,8 +145,10 @@ static void pending_read_result(struct pending_read *p, int err,
> > if (p->timeout_id > 0)
> > timeout_remove(p->timeout_id);
> >
> > - p->func(p->attrib, err, data, length, p->user_data);
> > + if (!p->disconn)
> > + p->func(p->attrib, err, data, length, p->user_data);
> >
> > + bt_att_unregister_disconnect(p->att, p->disconn_id);
> > free(p);
> > }
> >
> > @@ -156,8 +164,10 @@ static void pending_write_result(struct pending_write *p, int err)
> > if (p->timeout_id > 0)
> > timeout_remove(p->timeout_id);
> >
> > - p->func(p->attrib, err, p->user_data);
> > + if (!p->disconn)
> > + p->func(p->attrib, err, p->user_data);
> >
> > + bt_att_unregister_disconnect(p->att, p->disconn_id);
> > free(p);
> > }
>
> I wonder if it wouldn't be better to use a specific error to inform it
> the operation has been aborted e.g. -ECONNABORTED instead of
> duplicating the handling of disconnection, btw if we don't call the
> callback who is doing the cleanup in gatt-server.c, we still need to
> call async_read_op_destroy/async_write_op_destroy or that is taken
> care somewhere else?
>
> Also it would be great if we had a test in unit/test-gatt.c that
> covers such scenarios, e.g disconnect while read/write is pending.
Is there something not clear about the direction given here, or they
are understood but you still haven't had time to work on them?
> > @@ -1868,6 +1878,13 @@ bool gatt_db_attribute_set_fixed_length(struct gatt_db_attribute *attrib,
> > return true;
> > }
> >
> > +static void pending_read_cb(int err, void *user_data)
> > +{
> > + struct pending_read *p = user_data;
> > +
> > + p->disconn = 1;
> > +}
> > +
> > bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> > uint8_t opcode, struct bt_att *att,
> > gatt_db_attribute_read_t func, void *user_data)
> > @@ -1901,6 +1918,11 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
> > p->func = func;
> > p->user_data = user_data;
> >
> > + p->disconn = 0;
> > + p->disconn_id = bt_att_register_disconnect(att,
> > + pending_read_cb, p, NULL);
> > + p->att = att;
> > +
> > queue_push_tail(attrib->pending_reads, p);
> >
> > attrib->read_func(attrib, p->id, offset, opcode, att,
> > @@ -1956,6 +1978,13 @@ static bool write_timeout(void *user_data)
> > return false;
> > }
> >
> > +static void pending_write_cb(int err, void *user_data)
> > +{
> > + struct pending_write *p = user_data;
> > +
> > + p->disconn = 1;
> > +}
> > +
> > bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> > const uint8_t *value, size_t len,
> > uint8_t opcode, struct bt_att *att,
> > @@ -1995,6 +2024,11 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
> > p->func = func;
> > p->user_data = user_data;
> >
> > + p->disconn = 0;
> > + p->disconn_id = bt_att_register_disconnect(att,
> > + pending_write_cb, p, NULL);
> > + p->att = att;
> > +
> > queue_push_tail(attrib->pending_writes, p);
> >
> > attrib->write_func(attrib, p->id, offset, value, len, opcode,
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-10-13 21:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-08 19:00 [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout Bernie Conrad
2021-10-08 19:24 ` Luiz Augusto von Dentz
2021-10-13 21:38 ` Luiz Augusto von Dentz
2021-10-08 19:32 ` [BlueZ,v2] " bluez.test.bot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).