* [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 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).