linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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	[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).