* [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
@ 2021-09-28 23:00 Bernie Conrad
2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
0 siblings, 2 replies; 4+ messages in thread
From: Bernie Conrad @ 2021-09-28 23:00 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bernie Conrad
There is a current use after free possible on a gatt server if a client
disconnects while a WriteValue call is being processed with dbus.
This patch includes the addition of a pending disconnect callback to handle
cleanup better if a disconnect occurs during a write, an acquire write
or read operation using bt_att_register_disconnect with the cb.
v2: Fixed a bad call to pending_read_new
---
src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
1 file changed, 65 insertions(+), 50 deletions(-)
diff --git a/src/gatt-database.c b/src/gatt-database.c
index 1f7ce5f02..a03c579cf 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
return btd_adapter_find_device(adapter, &dst, dst_type);
}
+
+static void pending_op_free(void *data)
+{
+ struct pending_op *op = data;
+
+ if (op->owner_queue)
+ queue_remove(op->owner_queue, op);
+
+ free(op);
+}
+
+static void pending_disconnect_cb(int err, void *user_data)
+{
+ struct pending_op *op = user_data;
+
+ op->owner_queue = NULL;
+}
+
static struct pending_op *pending_ccc_new(struct bt_att *att,
struct gatt_db_attribute *attrib,
uint16_t value,
@@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
op->attrib = attrib;
op->link_type = link_type;
- return op;
-}
+ bt_att_register_disconnect(att,
+ pending_disconnect_cb,
+ op,
+ NULL);
-static void pending_op_free(void *data)
-{
- struct pending_op *op = data;
-
- if (op->owner_queue)
- queue_remove(op->owner_queue, op);
-
- free(op);
+ return op;
}
static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
@@ -2169,24 +2182,26 @@ done:
gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
}
-static struct pending_op *pending_read_new(struct btd_device *device,
+
+static struct pending_op *pending_read_new(struct bt_att *att,
struct queue *owner_queue,
struct gatt_db_attribute *attrib,
- unsigned int id, uint16_t offset,
- uint8_t link_type)
+ unsigned int id, uint16_t offset)
{
struct pending_op *op;
op = new0(struct pending_op, 1);
op->owner_queue = owner_queue;
- op->device = device;
+ op->device = att_get_device(att);
op->attrib = attrib;
op->id = id;
op->offset = offset;
- op->link_type = link_type;
+ op->link_type = bt_att_get_link_type(att);
queue_push_tail(owner_queue, op);
+ bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
+
return op;
}
@@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
dbus_message_iter_close_container(iter, &dict);
}
-static struct pending_op *send_read(struct btd_device *device,
+static struct pending_op *send_read(struct bt_att *att,
struct gatt_db_attribute *attrib,
GDBusProxy *proxy,
struct queue *owner_queue,
unsigned int id,
- uint16_t offset,
- uint8_t link_type)
+ uint16_t offset)
{
struct pending_op *op;
- op = pending_read_new(device, owner_queue, attrib, id, offset,
- link_type);
+ op = pending_read_new(att, owner_queue, attrib, id, offset);
if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
read_reply_cb, op, pending_op_free) == TRUE)
@@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
}
done:
- gatt_db_attribute_write_result(op->attrib, op->id, ecode);
+ // Make sure that only reply if the device is connected
+ if (btd_device_is_connected(op->device))
+ gatt_db_attribute_write_result(op->attrib, op->id, ecode);
}
-static struct pending_op *pending_write_new(struct btd_device *device,
+static struct pending_op *pending_write_new(struct bt_att *att,
struct queue *owner_queue,
struct gatt_db_attribute *attrib,
unsigned int id,
const uint8_t *value, size_t len,
- uint16_t offset, uint8_t link_type,
+ uint16_t offset,
bool is_characteristic,
bool prep_authorize)
{
@@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
op->data.iov_base = (uint8_t *) value;
op->data.iov_len = len;
- op->device = device;
+ op->device = att_get_device(att);
op->owner_queue = owner_queue;
op->attrib = attrib;
op->id = id;
op->offset = offset;
- op->link_type = link_type;
+ op->link_type = bt_att_get_link_type(att);
op->is_characteristic = is_characteristic;
op->prep_authorize = prep_authorize;
queue_push_tail(owner_queue, op);
+ bt_att_register_disconnect(att,
+ pending_disconnect_cb,
+ op, NULL);
+
return op;
}
-static struct pending_op *send_write(struct btd_device *device,
+static struct pending_op *send_write(struct bt_att *att,
struct gatt_db_attribute *attrib,
GDBusProxy *proxy,
struct queue *owner_queue,
unsigned int id,
const uint8_t *value, size_t len,
- uint16_t offset, uint8_t link_type,
+ uint16_t offset,
bool is_characteristic,
bool prep_authorize)
{
struct pending_op *op;
- op = pending_write_new(device, owner_queue, attrib, id, value, len,
- offset, link_type, is_characteristic,
+ op = pending_write_new(att, owner_queue, attrib, id, value, len,
+ offset, is_characteristic,
prep_authorize);
if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
@@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
}
static struct pending_op *acquire_write(struct external_chrc *chrc,
- struct btd_device *device,
+ struct bt_att *att,
struct gatt_db_attribute *attrib,
unsigned int id,
- const uint8_t *value, size_t len,
- uint8_t link_type)
+ const uint8_t *value, size_t len)
{
struct pending_op *op;
bool acquiring = !queue_isempty(chrc->pending_writes);
- op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
- len, 0, link_type, false, false);
+ op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
+ len, 0, false, false);
if (acquiring)
return op;
@@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
goto fail;
}
- if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
- offset, bt_att_get_link_type(att)))
+ if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
+ offset))
return;
fail:
@@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
if (!device_is_trusted(device) && !desc->prep_authorized &&
desc->req_prep_authorization)
- send_write(device, attrib, desc->proxy,
+ send_write(att, attrib, desc->proxy,
desc->pending_writes, id, value, len,
- offset, bt_att_get_link_type(att),
- false, true);
+ offset, false, true);
else
gatt_db_attribute_write_result(attrib, id, 0);
@@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
desc->prep_authorized = false;
- if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
- value, len, offset, bt_att_get_link_type(att), false,
- false))
+ if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
+ value, len, offset, false, false))
return;
fail:
@@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
goto fail;
}
- if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
- offset, bt_att_get_link_type(att)))
+ if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
+ offset))
return;
fail:
@@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
if (!device_is_trusted(device) && !chrc->prep_authorized &&
chrc->req_prep_authorization)
- send_write(device, attrib, chrc->proxy, queue,
+ send_write(att, attrib, chrc->proxy, queue,
id, value, len, offset,
- bt_att_get_link_type(att), true, true);
+ true, true);
else
gatt_db_attribute_write_result(attrib, id, 0);
@@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
}
if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
- if (acquire_write(chrc, device, attrib, id, value, len,
- bt_att_get_link_type(att)))
+ if (acquire_write(chrc, att, attrib, id, value, len))
return;
}
- if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
- offset, bt_att_get_link_type(att), false, false))
+ if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
+ offset, false, false))
return;
fail:
--
2.17.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* RE: [RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free
2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
@ 2021-09-28 23:36 ` bluez.test.bot
2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
1 sibling, 0 replies; 4+ messages in thread
From: bluez.test.bot @ 2021-09-28 23:36 UTC (permalink / raw)
To: linux-bluetooth, bernie
[-- Attachment #1: Type: text/plain, Size: 1216 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=554607
---Test result---
Test Summary:
CheckPatch PASS 1.56 seconds
GitLint FAIL 1.05 seconds
Prep - Setup ELL PASS 48.61 seconds
Build - Prep PASS 0.51 seconds
Build - Configure PASS 8.91 seconds
Build - Make PASS 207.06 seconds
Make Check PASS 9.64 seconds
Make Distcheck PASS 246.35 seconds
Build w/ext ELL - Configure PASS 9.02 seconds
Build w/ext ELL - Make PASS 195.19 seconds
Details
##############################
Test: GitLint - FAIL
Desc: Run gitlint with rule in .gitlint
Output:
[RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free
1: T1 Title exceeds max length (89>80): "[RFC,BlueZ,v2] Cleaning up gatt operations on a disconnect to fix possible use after free"
---
Regards,
Linux Bluetooth
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
@ 2021-09-29 21:05 ` Luiz Augusto von Dentz
2021-09-29 22:47 ` Luiz Augusto von Dentz
1 sibling, 1 reply; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 21:05 UTC (permalink / raw)
To: Bernie Conrad; +Cc: linux-bluetooth
Hi Bernie,
On Tue, Sep 28, 2021 at 4:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
>
> There is a current use after free possible on a gatt server if a client
> disconnects while a WriteValue call is being processed with dbus.
>
> This patch includes the addition of a pending disconnect callback to handle
> cleanup better if a disconnect occurs during a write, an acquire write
> or read operation using bt_att_register_disconnect with the cb.
>
> v2: Fixed a bad call to pending_read_new
>
> ---
> src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
> 1 file changed, 65 insertions(+), 50 deletions(-)
>
> diff --git a/src/gatt-database.c b/src/gatt-database.c
> index 1f7ce5f02..a03c579cf 100644
> --- a/src/gatt-database.c
> +++ b/src/gatt-database.c
> @@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
> return btd_adapter_find_device(adapter, &dst, dst_type);
> }
>
> +
> +static void pending_op_free(void *data)
> +{
> + struct pending_op *op = data;
> +
> + if (op->owner_queue)
> + queue_remove(op->owner_queue, op);
> +
> + free(op);
We might need to do something like the following:
https://gist.github.com/Vudentz/958d540591d1ae6a32d226ef8a0d6d54
Otherwise we risk the op being freeing when the op is completely
properly but when att is disconnected it still runs
pending_disconnect_cb causing a UAF.
> +}
> +
> +static void pending_disconnect_cb(int err, void *user_data)
> +{
> + struct pending_op *op = user_data;
> +
> + op->owner_queue = NULL;
> +}
> +
> static struct pending_op *pending_ccc_new(struct bt_att *att,
> struct gatt_db_attribute *attrib,
> uint16_t value,
> @@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
> op->attrib = attrib;
> op->link_type = link_type;
>
> - return op;
> -}
> + bt_att_register_disconnect(att,
> + pending_disconnect_cb,
> + op,
> + NULL);
>
> -static void pending_op_free(void *data)
> -{
> - struct pending_op *op = data;
> -
> - if (op->owner_queue)
> - queue_remove(op->owner_queue, op);
> -
> - free(op);
> + return op;
> }
>
> static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
> @@ -2169,24 +2182,26 @@ done:
> gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
> }
>
> -static struct pending_op *pending_read_new(struct btd_device *device,
> +
> +static struct pending_op *pending_read_new(struct bt_att *att,
> struct queue *owner_queue,
> struct gatt_db_attribute *attrib,
> - unsigned int id, uint16_t offset,
> - uint8_t link_type)
> + unsigned int id, uint16_t offset)
> {
> struct pending_op *op;
>
> op = new0(struct pending_op, 1);
>
> op->owner_queue = owner_queue;
> - op->device = device;
> + op->device = att_get_device(att);
> op->attrib = attrib;
> op->id = id;
> op->offset = offset;
> - op->link_type = link_type;
> + op->link_type = bt_att_get_link_type(att);
> queue_push_tail(owner_queue, op);
>
> + bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
> +
> return op;
> }
>
> @@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
> dbus_message_iter_close_container(iter, &dict);
> }
>
> -static struct pending_op *send_read(struct btd_device *device,
> +static struct pending_op *send_read(struct bt_att *att,
> struct gatt_db_attribute *attrib,
> GDBusProxy *proxy,
> struct queue *owner_queue,
> unsigned int id,
> - uint16_t offset,
> - uint8_t link_type)
> + uint16_t offset)
> {
> struct pending_op *op;
>
> - op = pending_read_new(device, owner_queue, attrib, id, offset,
> - link_type);
> + op = pending_read_new(att, owner_queue, attrib, id, offset);
>
> if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
> read_reply_cb, op, pending_op_free) == TRUE)
> @@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
> }
>
> done:
> - gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> + // Make sure that only reply if the device is connected
> + if (btd_device_is_connected(op->device))
> + gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> }
>
> -static struct pending_op *pending_write_new(struct btd_device *device,
> +static struct pending_op *pending_write_new(struct bt_att *att,
> struct queue *owner_queue,
> struct gatt_db_attribute *attrib,
> unsigned int id,
> const uint8_t *value, size_t len,
> - uint16_t offset, uint8_t link_type,
> + uint16_t offset,
> bool is_characteristic,
> bool prep_authorize)
> {
> @@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
> op->data.iov_base = (uint8_t *) value;
> op->data.iov_len = len;
>
> - op->device = device;
> + op->device = att_get_device(att);
> op->owner_queue = owner_queue;
> op->attrib = attrib;
> op->id = id;
> op->offset = offset;
> - op->link_type = link_type;
> + op->link_type = bt_att_get_link_type(att);
> op->is_characteristic = is_characteristic;
> op->prep_authorize = prep_authorize;
> queue_push_tail(owner_queue, op);
>
> + bt_att_register_disconnect(att,
> + pending_disconnect_cb,
> + op, NULL);
> +
> return op;
> }
>
> -static struct pending_op *send_write(struct btd_device *device,
> +static struct pending_op *send_write(struct bt_att *att,
> struct gatt_db_attribute *attrib,
> GDBusProxy *proxy,
> struct queue *owner_queue,
> unsigned int id,
> const uint8_t *value, size_t len,
> - uint16_t offset, uint8_t link_type,
> + uint16_t offset,
> bool is_characteristic,
> bool prep_authorize)
> {
> struct pending_op *op;
>
> - op = pending_write_new(device, owner_queue, attrib, id, value, len,
> - offset, link_type, is_characteristic,
> + op = pending_write_new(att, owner_queue, attrib, id, value, len,
> + offset, is_characteristic,
> prep_authorize);
>
> if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
> @@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
> }
>
> static struct pending_op *acquire_write(struct external_chrc *chrc,
> - struct btd_device *device,
> + struct bt_att *att,
> struct gatt_db_attribute *attrib,
> unsigned int id,
> - const uint8_t *value, size_t len,
> - uint8_t link_type)
> + const uint8_t *value, size_t len)
> {
> struct pending_op *op;
> bool acquiring = !queue_isempty(chrc->pending_writes);
>
> - op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
> - len, 0, link_type, false, false);
> + op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
> + len, 0, false, false);
>
> if (acquiring)
> return op;
> @@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
> goto fail;
> }
>
> - if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
> - offset, bt_att_get_link_type(att)))
> + if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
> + offset))
> return;
>
> fail:
> @@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> if (!device_is_trusted(device) && !desc->prep_authorized &&
> desc->req_prep_authorization)
> - send_write(device, attrib, desc->proxy,
> + send_write(att, attrib, desc->proxy,
> desc->pending_writes, id, value, len,
> - offset, bt_att_get_link_type(att),
> - false, true);
> + offset, false, true);
> else
> gatt_db_attribute_write_result(attrib, id, 0);
>
> @@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
> desc->prep_authorized = false;
>
> - if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
> - value, len, offset, bt_att_get_link_type(att), false,
> - false))
> + if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
> + value, len, offset, false, false))
> return;
>
> fail:
> @@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
> goto fail;
> }
>
> - if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
> - offset, bt_att_get_link_type(att)))
> + if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
> + offset))
> return;
>
> fail:
> @@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> if (!device_is_trusted(device) && !chrc->prep_authorized &&
> chrc->req_prep_authorization)
> - send_write(device, attrib, chrc->proxy, queue,
> + send_write(att, attrib, chrc->proxy, queue,
> id, value, len, offset,
> - bt_att_get_link_type(att), true, true);
> + true, true);
> else
> gatt_db_attribute_write_result(attrib, id, 0);
>
> @@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> }
>
> if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
> - if (acquire_write(chrc, device, attrib, id, value, len,
> - bt_att_get_link_type(att)))
> + if (acquire_write(chrc, att, attrib, id, value, len))
> return;
> }
>
> - if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
> - offset, bt_att_get_link_type(att), false, false))
> + if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
> + offset, false, false))
> return;
>
> fail:
> --
> 2.17.1
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free
2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
@ 2021-09-29 22:47 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 4+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 22:47 UTC (permalink / raw)
To: Bernie Conrad; +Cc: linux-bluetooth
Hi Bernie,
On Wed, Sep 29, 2021 at 2:05 PM Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Bernie,
>
> On Tue, Sep 28, 2021 at 4:01 PM Bernie Conrad <bernie@allthenticate.net> wrote:
> >
> > There is a current use after free possible on a gatt server if a client
> > disconnects while a WriteValue call is being processed with dbus.
> >
> > This patch includes the addition of a pending disconnect callback to handle
> > cleanup better if a disconnect occurs during a write, an acquire write
> > or read operation using bt_att_register_disconnect with the cb.
> >
> > v2: Fixed a bad call to pending_read_new
> >
> > ---
> > src/gatt-database.c | 115 +++++++++++++++++++++++++-------------------
> > 1 file changed, 65 insertions(+), 50 deletions(-)
> >
> > diff --git a/src/gatt-database.c b/src/gatt-database.c
> > index 1f7ce5f02..a03c579cf 100644
> > --- a/src/gatt-database.c
> > +++ b/src/gatt-database.c
> > @@ -933,6 +933,24 @@ static struct btd_device *att_get_device(struct bt_att *att)
> > return btd_adapter_find_device(adapter, &dst, dst_type);
> > }
> >
> > +
> > +static void pending_op_free(void *data)
> > +{
> > + struct pending_op *op = data;
> > +
> > + if (op->owner_queue)
> > + queue_remove(op->owner_queue, op);
> > +
> > + free(op);
>
> We might need to do something like the following:
>
> https://gist.github.com/Vudentz/958d540591d1ae6a32d226ef8a0d6d54
>
> Otherwise we risk the op being freeing when the op is completely
> properly but when att is disconnected it still runs
> pending_disconnect_cb causing a UAF.
>
> > +}
> > +
> > +static void pending_disconnect_cb(int err, void *user_data)
> > +{
> > + struct pending_op *op = user_data;
> > +
> > + op->owner_queue = NULL;
> > +}
> > +
> > static struct pending_op *pending_ccc_new(struct bt_att *att,
> > struct gatt_db_attribute *attrib,
> > uint16_t value,
> > @@ -956,17 +974,12 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
> > op->attrib = attrib;
> > op->link_type = link_type;
> >
> > - return op;
> > -}
> > + bt_att_register_disconnect(att,
> > + pending_disconnect_cb,
> > + op,
> > + NULL);
> >
> > -static void pending_op_free(void *data)
> > -{
> > - struct pending_op *op = data;
> > -
> > - if (op->owner_queue)
> > - queue_remove(op->owner_queue, op);
> > -
> > - free(op);
> > + return op;
> > }
> >
> > static void gatt_ccc_write_cb(struct gatt_db_attribute *attrib,
> > @@ -2169,24 +2182,26 @@ done:
> > gatt_db_attribute_read_result(op->attrib, op->id, ecode, value, len);
> > }
> >
> > -static struct pending_op *pending_read_new(struct btd_device *device,
> > +
> > +static struct pending_op *pending_read_new(struct bt_att *att,
> > struct queue *owner_queue,
> > struct gatt_db_attribute *attrib,
> > - unsigned int id, uint16_t offset,
> > - uint8_t link_type)
> > + unsigned int id, uint16_t offset)
> > {
> > struct pending_op *op;
> >
> > op = new0(struct pending_op, 1);
> >
> > op->owner_queue = owner_queue;
> > - op->device = device;
> > + op->device = att_get_device(att);
> > op->attrib = attrib;
> > op->id = id;
> > op->offset = offset;
> > - op->link_type = link_type;
> > + op->link_type = bt_att_get_link_type(att);
> > queue_push_tail(owner_queue, op);
> >
> > + bt_att_register_disconnect(att, pending_disconnect_cb, op, NULL);
> > +
> > return op;
> > }
> >
> > @@ -2243,18 +2258,16 @@ static void read_setup_cb(DBusMessageIter *iter, void *user_data)
> > dbus_message_iter_close_container(iter, &dict);
> > }
> >
> > -static struct pending_op *send_read(struct btd_device *device,
> > +static struct pending_op *send_read(struct bt_att *att,
> > struct gatt_db_attribute *attrib,
> > GDBusProxy *proxy,
> > struct queue *owner_queue,
> > unsigned int id,
> > - uint16_t offset,
> > - uint8_t link_type)
> > + uint16_t offset)
> > {
> > struct pending_op *op;
> >
> > - op = pending_read_new(device, owner_queue, attrib, id, offset,
> > - link_type);
> > + op = pending_read_new(att, owner_queue, attrib, id, offset);
> >
> > if (g_dbus_proxy_method_call(proxy, "ReadValue", read_setup_cb,
> > read_reply_cb, op, pending_op_free) == TRUE)
> > @@ -2337,15 +2350,17 @@ static void write_reply_cb(DBusMessage *message, void *user_data)
> > }
> >
> > done:
> > - gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> > + // Make sure that only reply if the device is connected
> > + if (btd_device_is_connected(op->device))
> > + gatt_db_attribute_write_result(op->attrib, op->id, ecode);
> > }
> >
> > -static struct pending_op *pending_write_new(struct btd_device *device,
> > +static struct pending_op *pending_write_new(struct bt_att *att,
> > struct queue *owner_queue,
> > struct gatt_db_attribute *attrib,
> > unsigned int id,
> > const uint8_t *value, size_t len,
> > - uint16_t offset, uint8_t link_type,
> > + uint16_t offset,
> > bool is_characteristic,
> > bool prep_authorize)
> > {
> > @@ -2356,33 +2371,37 @@ static struct pending_op *pending_write_new(struct btd_device *device,
> > op->data.iov_base = (uint8_t *) value;
> > op->data.iov_len = len;
> >
> > - op->device = device;
> > + op->device = att_get_device(att);
> > op->owner_queue = owner_queue;
> > op->attrib = attrib;
> > op->id = id;
> > op->offset = offset;
> > - op->link_type = link_type;
> > + op->link_type = bt_att_get_link_type(att);
> > op->is_characteristic = is_characteristic;
> > op->prep_authorize = prep_authorize;
> > queue_push_tail(owner_queue, op);
> >
> > + bt_att_register_disconnect(att,
> > + pending_disconnect_cb,
> > + op, NULL);
> > +
> > return op;
> > }
> >
> > -static struct pending_op *send_write(struct btd_device *device,
> > +static struct pending_op *send_write(struct bt_att *att,
> > struct gatt_db_attribute *attrib,
> > GDBusProxy *proxy,
> > struct queue *owner_queue,
> > unsigned int id,
> > const uint8_t *value, size_t len,
> > - uint16_t offset, uint8_t link_type,
> > + uint16_t offset,
> > bool is_characteristic,
> > bool prep_authorize)
> > {
> > struct pending_op *op;
> >
> > - op = pending_write_new(device, owner_queue, attrib, id, value, len,
> > - offset, link_type, is_characteristic,
> > + op = pending_write_new(att, owner_queue, attrib, id, value, len,
> > + offset, is_characteristic,
> > prep_authorize);
> >
> > if (g_dbus_proxy_method_call(proxy, "WriteValue", write_setup_cb,
> > @@ -2558,17 +2577,16 @@ static void acquire_write_setup(DBusMessageIter *iter, void *user_data)
> > }
> >
> > static struct pending_op *acquire_write(struct external_chrc *chrc,
> > - struct btd_device *device,
> > + struct bt_att *att,
> > struct gatt_db_attribute *attrib,
> > unsigned int id,
> > - const uint8_t *value, size_t len,
> > - uint8_t link_type)
> > + const uint8_t *value, size_t len)
> > {
> > struct pending_op *op;
> > bool acquiring = !queue_isempty(chrc->pending_writes);
> >
> > - op = pending_write_new(device, chrc->pending_writes, attrib, id, value,
> > - len, 0, link_type, false, false);
> > + op = pending_write_new(att, chrc->pending_writes, attrib, id, value,
> > + len, 0, false, false);
> >
> > if (acquiring)
> > return op;
> > @@ -2851,8 +2869,8 @@ static void desc_read_cb(struct gatt_db_attribute *attrib,
> > goto fail;
> > }
> >
> > - if (send_read(device, attrib, desc->proxy, desc->pending_reads, id,
> > - offset, bt_att_get_link_type(att)))
> > + if (send_read(att, attrib, desc->proxy, desc->pending_reads, id,
> > + offset))
> > return;
> >
> > fail:
> > @@ -2883,10 +2901,9 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> > if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> > if (!device_is_trusted(device) && !desc->prep_authorized &&
> > desc->req_prep_authorization)
> > - send_write(device, attrib, desc->proxy,
> > + send_write(att, attrib, desc->proxy,
> > desc->pending_writes, id, value, len,
> > - offset, bt_att_get_link_type(att),
> > - false, true);
> > + offset, false, true);
> > else
> > gatt_db_attribute_write_result(attrib, id, 0);
> >
> > @@ -2896,9 +2913,8 @@ static void desc_write_cb(struct gatt_db_attribute *attrib,
> > if (opcode == BT_ATT_OP_EXEC_WRITE_REQ)
> > desc->prep_authorized = false;
> >
> > - if (send_write(device, attrib, desc->proxy, desc->pending_writes, id,
> > - value, len, offset, bt_att_get_link_type(att), false,
> > - false))
> > + if (send_write(att, attrib, desc->proxy, desc->pending_writes, id,
> > + value, len, offset, false, false))
> > return;
> >
> > fail:
> > @@ -2977,8 +2993,8 @@ static void chrc_read_cb(struct gatt_db_attribute *attrib,
> > goto fail;
> > }
> >
> > - if (send_read(device, attrib, chrc->proxy, chrc->pending_reads, id,
> > - offset, bt_att_get_link_type(att)))
> > + if (send_read(att, attrib, chrc->proxy, chrc->pending_reads, id,
> > + offset))
> > return;
> >
> > fail:
> > @@ -3016,9 +3032,9 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> > if (opcode == BT_ATT_OP_PREP_WRITE_REQ) {
> > if (!device_is_trusted(device) && !chrc->prep_authorized &&
> > chrc->req_prep_authorization)
> > - send_write(device, attrib, chrc->proxy, queue,
> > + send_write(att, attrib, chrc->proxy, queue,
> > id, value, len, offset,
> > - bt_att_get_link_type(att), true, true);
> > + true, true);
> > else
> > gatt_db_attribute_write_result(attrib, id, 0);
> >
> > @@ -3039,13 +3055,12 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
> > }
> >
> > if (g_dbus_proxy_get_property(chrc->proxy, "WriteAcquired", &iter)) {
> > - if (acquire_write(chrc, device, attrib, id, value, len,
> > - bt_att_get_link_type(att)))
> > + if (acquire_write(chrc, att, attrib, id, value, len))
> > return;
> > }
> >
> > - if (send_write(device, attrib, chrc->proxy, queue, id, value, len,
> > - offset, bt_att_get_link_type(att), false, false))
> > + if (send_write(att, attrib, chrc->proxy, queue, id, value, len,
> > + offset, false, false))
> > return;
> >
> > fail:
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz
I went ahead and applied this with my changes on top, thanks.
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-09-29 22:48 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 23:00 [RFC PATCH BlueZ v2] Cleaning up gatt operations on a disconnect to fix possible use after free Bernie Conrad
2021-09-28 23:36 ` [RFC,BlueZ,v2] " bluez.test.bot
2021-09-29 21:05 ` [RFC PATCH BlueZ v2] " Luiz Augusto von Dentz
2021-09-29 22:47 ` Luiz Augusto von Dentz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.