| Message ID | ZiwwPmCvU25YzWek@neat (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,next] Bluetooth: hci_conn, hci_sync: Use __counted_by() in multiple structs and avoid -Wfamnae warnings | expand |
On Fri, Apr 26, 2024 at 04:52:46PM -0600, Gustavo A. R. Silva wrote: > Prepare for the coming implementation by GCC and Clang of the > __counted_by attribute. Flexible array members annotated with > __counted_by can have their accesses bounds-checked at run-time > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE > (for strcpy/memcpy-family functions). > > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are > getting ready to enable it globally. > > So, use the `DEFINE_FLEX()` helper for multiple on-stack definitions > of a flexible structure where the size of the flexible-array member > is known at compile-time, and refactor the rest of the code, > accordingly. > > Notice that, due to the use of `__counted_by()` in `struct > hci_cp_le_create_cis`, the for loop in function `hci_cs_le_create_cis()` > had to be modified. Once the index `i`, through which `cp->cis[i]` is > accessed, falls in the interval [0, cp->num_cis), `cp->num_cis` cannot > be decremented all the way down to zero while accessing `cp->cis[]`: > > net/bluetooth/hci_event.c:4310: > 4310 for (i = 0; cp->num_cis; cp->num_cis--, i++) { > ... > 4314 handle = __le16_to_cpu(cp->cis[i].cis_handle); > > otherwise, only half (one iteration before `cp->num_cis == i`) or half > plus one (one iteration before `cp->num_cis < i`) of the items in the > array will be accessed before running into an out-of-bounds issue. So, > in order to avoid this, set `cp->num_cis` to zero just after the for > loop. > > Also, make use of `aux_num_cis` variable to update `cmd->num_cis` after > a `list_for_each_entry_rcu()` loop. > > With these changes, fix the following warnings: > net/bluetooth/hci_sync.c:1239:56: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > net/bluetooth/hci_sync.c:1415:51: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > net/bluetooth/hci_sync.c:1731:51: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > net/bluetooth/hci_sync.c:6497:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > > Link: https://github.com/KSPP/linux/issues/202 > Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> > --- > Changes in v2: > - Update `cmd->num_cis` after `list_for_each_entry_rcu()` loop. > > v1: > - Link: https://lore.kernel.org/linux-hardening/ZiwqqZCa7PK9bzCX@neat/ > > include/net/bluetooth/hci.h | 8 ++-- > net/bluetooth/hci_event.c | 3 +- > net/bluetooth/hci_sync.c | 84 +++++++++++++++---------------------- > 3 files changed, 40 insertions(+), 55 deletions(-) > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > index fe23e862921d..c4c6b8810701 100644 > --- a/include/net/bluetooth/hci.h > +++ b/include/net/bluetooth/hci.h > @@ -2026,7 +2026,7 @@ struct hci_cp_le_set_ext_adv_data { > __u8 operation; > __u8 frag_pref; > __u8 length; > - __u8 data[]; > + __u8 data[] __counted_by(length); > } __packed; I noticed some of the other structs here aren't flexible arrays, so it made me go take a look at these ones. I see that the only user of struct hci_cp_le_set_ext_adv_data uses a fixed-size array: struct { struct hci_cp_le_set_ext_adv_data cp; u8 data[HCI_MAX_EXT_AD_LENGTH]; } pdu; Let's just change this from a flex array to a fixed-size array? > > #define HCI_OP_LE_SET_EXT_SCAN_RSP_DATA 0x2038 > @@ -2035,7 +2035,7 @@ struct hci_cp_le_set_ext_scan_rsp_data { > __u8 operation; > __u8 frag_pref; > __u8 length; > - __u8 data[]; > + __u8 data[] __counted_by(length); > } __packed; Same for this one: struct { struct hci_cp_le_set_ext_scan_rsp_data cp; u8 data[HCI_MAX_EXT_AD_LENGTH]; } pdu; > > #define HCI_OP_LE_SET_EXT_ADV_ENABLE 0x2039 > @@ -2061,7 +2061,7 @@ struct hci_cp_le_set_per_adv_data { > __u8 handle; > __u8 operation; > __u8 length; > - __u8 data[]; > + __u8 data[] __counted_by(length); > } __packed; And this one. :P struct { struct hci_cp_le_set_per_adv_data cp; u8 data[HCI_MAX_PER_AD_LENGTH]; } pdu; > > #define HCI_OP_LE_SET_PER_ADV_ENABLE 0x2040 > @@ -2162,7 +2162,7 @@ struct hci_cis { > > struct hci_cp_le_create_cis { > __u8 num_cis; > - struct hci_cis cis[]; > + struct hci_cis cis[] __counted_by(num_cis); > } __packed; This one isn't as obvious, so I'd say keep your changes for this one. > > #define HCI_OP_LE_REMOVE_CIG 0x2065 > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c > index 9a38e155537e..9a7ca084302e 100644 > --- a/net/bluetooth/hci_event.c > +++ b/net/bluetooth/hci_event.c > @@ -4307,7 +4307,7 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status) > hci_dev_lock(hdev); > > /* Remove connection if command failed */ > - for (i = 0; cp->num_cis; cp->num_cis--, i++) { > + for (i = 0; i < cp->num_cis; i++) { > struct hci_conn *conn; > u16 handle; > > @@ -4323,6 +4323,7 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status) > hci_conn_del(conn); > } > } > + cp->num_cis = 0; Yeah, this loop never leaves early, and if it did, it processing the array forward, so having num_cis reduced during the loop iteration doesn't make sense. What you have looks right to me! > > if (pending) > hci_le_create_cis_pending(hdev); > diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c > index 9092b4d59545..6e15594d3565 100644 > --- a/net/bluetooth/hci_sync.c > +++ b/net/bluetooth/hci_sync.c > @@ -1235,31 +1235,27 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) > > static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance) > { > - struct { > - struct hci_cp_le_set_ext_scan_rsp_data cp; > - u8 data[HCI_MAX_EXT_AD_LENGTH]; > - } pdu; > + DEFINE_FLEX(struct hci_cp_le_set_ext_scan_rsp_data, pdu, data, length, > + HCI_MAX_EXT_AD_LENGTH); > u8 len; > struct adv_info *adv = NULL; > int err; > > - memset(&pdu, 0, sizeof(pdu)); These become much easier, just: struct hci_cp_le_set_ext_scan_rsp_data cp = { }; etc...
Hello: This patch was applied to bluetooth/bluetooth-next.git (master) by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>: On Fri, 26 Apr 2024 16:52:46 -0600 you wrote: > Prepare for the coming implementation by GCC and Clang of the > __counted_by attribute. Flexible array members annotated with > __counted_by can have their accesses bounds-checked at run-time > via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE > (for strcpy/memcpy-family functions). > > Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are > getting ready to enable it globally. > > [...] Here is the summary with links: - [v2,next] Bluetooth: hci_conn, hci_sync: Use __counted_by() in multiple structs and avoid -Wfamnae warnings https://git.kernel.org/bluetooth/bluetooth-next/c/4a4008a5b0e3 You are awesome, thank you!
>> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h >> index fe23e862921d..c4c6b8810701 100644 >> --- a/include/net/bluetooth/hci.h >> +++ b/include/net/bluetooth/hci.h >> @@ -2026,7 +2026,7 @@ struct hci_cp_le_set_ext_adv_data { >> __u8 operation; >> __u8 frag_pref; >> __u8 length; >> - __u8 data[]; >> + __u8 data[] __counted_by(length); >> } __packed; > > I noticed some of the other structs here aren't flexible arrays, so it > made me go take a look at these ones. I see that the only user of struct > hci_cp_le_set_ext_adv_data uses a fixed-size array: > > struct { > struct hci_cp_le_set_ext_adv_data cp; > u8 data[HCI_MAX_EXT_AD_LENGTH]; > } pdu; > > Let's just change this from a flex array to a fixed-size array? mmh... not sure about this. It would basically mean reverting this commit: c9ed0a707730 ("Bluetooth: Fix Set Extended (Scan Response) Data") -- Gustavo
On Mon, Apr 29, 2024 at 01:50:46PM -0600, Gustavo A. R. Silva wrote: > > > > diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h > > > index fe23e862921d..c4c6b8810701 100644 > > > --- a/include/net/bluetooth/hci.h > > > +++ b/include/net/bluetooth/hci.h > > > @@ -2026,7 +2026,7 @@ struct hci_cp_le_set_ext_adv_data { > > > __u8 operation; > > > __u8 frag_pref; > > > __u8 length; > > > - __u8 data[]; > > > + __u8 data[] __counted_by(length); > > > } __packed; > > > > I noticed some of the other structs here aren't flexible arrays, so it > > made me go take a look at these ones. I see that the only user of struct > > hci_cp_le_set_ext_adv_data uses a fixed-size array: > > > > struct { > > struct hci_cp_le_set_ext_adv_data cp; > > u8 data[HCI_MAX_EXT_AD_LENGTH]; > > } pdu; > > > > Let's just change this from a flex array to a fixed-size array? > > mmh... not sure about this. It would basically mean reverting this commit: > > c9ed0a707730 ("Bluetooth: Fix Set Extended (Scan Response) Data") That change doesn't seem to need to make them flex arrays, though -- there's no savings at all (the same amount is stack allocated). Anyway, not a big deal, I guess. It's an improvement to be using __counted_by, so good! :) Reviewed-by: Kees Cook <keescook@chromium.org>
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h index fe23e862921d..c4c6b8810701 100644 --- a/include/net/bluetooth/hci.h +++ b/include/net/bluetooth/hci.h @@ -2026,7 +2026,7 @@ struct hci_cp_le_set_ext_adv_data { __u8 operation; __u8 frag_pref; __u8 length; - __u8 data[]; + __u8 data[] __counted_by(length); } __packed; #define HCI_OP_LE_SET_EXT_SCAN_RSP_DATA 0x2038 @@ -2035,7 +2035,7 @@ struct hci_cp_le_set_ext_scan_rsp_data { __u8 operation; __u8 frag_pref; __u8 length; - __u8 data[]; + __u8 data[] __counted_by(length); } __packed; #define HCI_OP_LE_SET_EXT_ADV_ENABLE 0x2039 @@ -2061,7 +2061,7 @@ struct hci_cp_le_set_per_adv_data { __u8 handle; __u8 operation; __u8 length; - __u8 data[]; + __u8 data[] __counted_by(length); } __packed; #define HCI_OP_LE_SET_PER_ADV_ENABLE 0x2040 @@ -2162,7 +2162,7 @@ struct hci_cis { struct hci_cp_le_create_cis { __u8 num_cis; - struct hci_cis cis[]; + struct hci_cis cis[] __counted_by(num_cis); } __packed; #define HCI_OP_LE_REMOVE_CIG 0x2065 diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 9a38e155537e..9a7ca084302e 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -4307,7 +4307,7 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status) hci_dev_lock(hdev); /* Remove connection if command failed */ - for (i = 0; cp->num_cis; cp->num_cis--, i++) { + for (i = 0; i < cp->num_cis; i++) { struct hci_conn *conn; u16 handle; @@ -4323,6 +4323,7 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev, u8 status) hci_conn_del(conn); } } + cp->num_cis = 0; if (pending) hci_le_create_cis_pending(hdev); diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 9092b4d59545..6e15594d3565 100644 --- a/net/bluetooth/hci_sync.c +++ b/net/bluetooth/hci_sync.c @@ -1235,31 +1235,27 @@ int hci_setup_ext_adv_instance_sync(struct hci_dev *hdev, u8 instance) static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance) { - struct { - struct hci_cp_le_set_ext_scan_rsp_data cp; - u8 data[HCI_MAX_EXT_AD_LENGTH]; - } pdu; + DEFINE_FLEX(struct hci_cp_le_set_ext_scan_rsp_data, pdu, data, length, + HCI_MAX_EXT_AD_LENGTH); u8 len; struct adv_info *adv = NULL; int err; - memset(&pdu, 0, sizeof(pdu)); - if (instance) { adv = hci_find_adv_instance(hdev, instance); if (!adv || !adv->scan_rsp_changed) return 0; } - len = eir_create_scan_rsp(hdev, instance, pdu.data); + len = eir_create_scan_rsp(hdev, instance, pdu->data); - pdu.cp.handle = instance; - pdu.cp.length = len; - pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE; - pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG; + pdu->handle = instance; + pdu->length = len; + pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE; + pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG; err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_SCAN_RSP_DATA, - sizeof(pdu.cp) + len, &pdu.cp, + struct_size(pdu, data, len), pdu, HCI_CMD_TIMEOUT); if (err) return err; @@ -1267,7 +1263,7 @@ static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instance) if (adv) { adv->scan_rsp_changed = false; } else { - memcpy(hdev->scan_rsp_data, pdu.data, len); + memcpy(hdev->scan_rsp_data, pdu->data, len); hdev->scan_rsp_data_len = len; } @@ -1411,14 +1407,10 @@ static int hci_set_per_adv_params_sync(struct hci_dev *hdev, u8 instance, static int hci_set_per_adv_data_sync(struct hci_dev *hdev, u8 instance) { - struct { - struct hci_cp_le_set_per_adv_data cp; - u8 data[HCI_MAX_PER_AD_LENGTH]; - } pdu; + DEFINE_FLEX(struct hci_cp_le_set_per_adv_data, pdu, data, length, + HCI_MAX_PER_AD_LENGTH); u8 len; - memset(&pdu, 0, sizeof(pdu)); - if (instance) { struct adv_info *adv = hci_find_adv_instance(hdev, instance); @@ -1426,14 +1418,14 @@ static int hci_set_per_adv_data_sync(struct hci_dev *hdev, u8 instance) return 0; } - len = eir_create_per_adv_data(hdev, instance, pdu.data); + len = eir_create_per_adv_data(hdev, instance, pdu->data); - pdu.cp.length = len; - pdu.cp.handle = instance; - pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE; + pdu->length = len; + pdu->handle = instance; + pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE; return __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_PER_ADV_DATA, - sizeof(pdu.cp) + len, &pdu, + struct_size(pdu, data, len), pdu, HCI_CMD_TIMEOUT); } @@ -1727,31 +1719,27 @@ int hci_le_terminate_big_sync(struct hci_dev *hdev, u8 handle, u8 reason) static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance) { - struct { - struct hci_cp_le_set_ext_adv_data cp; - u8 data[HCI_MAX_EXT_AD_LENGTH]; - } pdu; + DEFINE_FLEX(struct hci_cp_le_set_ext_adv_data, pdu, data, length, + HCI_MAX_EXT_AD_LENGTH); u8 len; struct adv_info *adv = NULL; int err; - memset(&pdu, 0, sizeof(pdu)); - if (instance) { adv = hci_find_adv_instance(hdev, instance); if (!adv || !adv->adv_data_changed) return 0; } - len = eir_create_adv_data(hdev, instance, pdu.data); + len = eir_create_adv_data(hdev, instance, pdu->data); - pdu.cp.length = len; - pdu.cp.handle = instance; - pdu.cp.operation = LE_SET_ADV_DATA_OP_COMPLETE; - pdu.cp.frag_pref = LE_SET_ADV_DATA_NO_FRAG; + pdu->length = len; + pdu->handle = instance; + pdu->operation = LE_SET_ADV_DATA_OP_COMPLETE; + pdu->frag_pref = LE_SET_ADV_DATA_NO_FRAG; err = __hci_cmd_sync_status(hdev, HCI_OP_LE_SET_EXT_ADV_DATA, - sizeof(pdu.cp) + len, &pdu.cp, + struct_size(pdu, data, len), pdu, HCI_CMD_TIMEOUT); if (err) return err; @@ -1760,7 +1748,7 @@ static int hci_set_ext_adv_data_sync(struct hci_dev *hdev, u8 instance) if (adv) { adv->adv_data_changed = false; } else { - memcpy(hdev->adv_data, pdu.data, len); + memcpy(hdev->adv_data, pdu->data, len); hdev->adv_data_len = len; } @@ -6496,10 +6484,8 @@ static int hci_le_create_conn_sync(struct hci_dev *hdev, void *data) int hci_le_create_cis_sync(struct hci_dev *hdev) { - struct { - struct hci_cp_le_create_cis cp; - struct hci_cis cis[0x1f]; - } cmd; + DEFINE_FLEX(struct hci_cp_le_create_cis, cmd, cis, num_cis, 0x1f); + size_t aux_num_cis = 0; struct hci_conn *conn; u8 cig = BT_ISO_QOS_CIG_UNSET; @@ -6526,8 +6512,6 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) * remains pending. */ - memset(&cmd, 0, sizeof(cmd)); - hci_dev_lock(hdev); rcu_read_lock(); @@ -6564,7 +6548,7 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) goto done; list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) { - struct hci_cis *cis = &cmd.cis[cmd.cp.num_cis]; + struct hci_cis *cis = &cmd->cis[aux_num_cis]; if (hci_conn_check_create_cis(conn) || conn->iso_qos.ucast.cig != cig) @@ -6573,25 +6557,25 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) set_bit(HCI_CONN_CREATE_CIS, &conn->flags); cis->acl_handle = cpu_to_le16(conn->parent->handle); cis->cis_handle = cpu_to_le16(conn->handle); - cmd.cp.num_cis++; + aux_num_cis++; - if (cmd.cp.num_cis >= ARRAY_SIZE(cmd.cis)) + if (aux_num_cis >= 0x1f) break; } + cmd->num_cis = aux_num_cis; done: rcu_read_unlock(); hci_dev_unlock(hdev); - if (!cmd.cp.num_cis) + if (!aux_num_cis) return 0; /* Wait for HCI_LE_CIS_Established */ return __hci_cmd_sync_status_sk(hdev, HCI_OP_LE_CREATE_CIS, - sizeof(cmd.cp) + sizeof(cmd.cis[0]) * - cmd.cp.num_cis, &cmd, - HCI_EVT_LE_CIS_ESTABLISHED, + struct_size(cmd, cis, cmd->num_cis), + cmd, HCI_EVT_LE_CIS_ESTABLISHED, conn->conn_timeout, NULL); }
Prepare for the coming implementation by GCC and Clang of the __counted_by attribute. Flexible array members annotated with __counted_by can have their accesses bounds-checked at run-time via CONFIG_UBSAN_BOUNDS (for array indexing) and CONFIG_FORTIFY_SOURCE (for strcpy/memcpy-family functions). Also, -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. So, use the `DEFINE_FLEX()` helper for multiple on-stack definitions of a flexible structure where the size of the flexible-array member is known at compile-time, and refactor the rest of the code, accordingly. Notice that, due to the use of `__counted_by()` in `struct hci_cp_le_create_cis`, the for loop in function `hci_cs_le_create_cis()` had to be modified. Once the index `i`, through which `cp->cis[i]` is accessed, falls in the interval [0, cp->num_cis), `cp->num_cis` cannot be decremented all the way down to zero while accessing `cp->cis[]`: net/bluetooth/hci_event.c:4310: 4310 for (i = 0; cp->num_cis; cp->num_cis--, i++) { ... 4314 handle = __le16_to_cpu(cp->cis[i].cis_handle); otherwise, only half (one iteration before `cp->num_cis == i`) or half plus one (one iteration before `cp->num_cis < i`) of the items in the array will be accessed before running into an out-of-bounds issue. So, in order to avoid this, set `cp->num_cis` to zero just after the for loop. Also, make use of `aux_num_cis` variable to update `cmd->num_cis` after a `list_for_each_entry_rcu()` loop. With these changes, fix the following warnings: net/bluetooth/hci_sync.c:1239:56: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] net/bluetooth/hci_sync.c:1415:51: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] net/bluetooth/hci_sync.c:1731:51: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] net/bluetooth/hci_sync.c:6497:45: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] Link: https://github.com/KSPP/linux/issues/202 Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- Changes in v2: - Update `cmd->num_cis` after `list_for_each_entry_rcu()` loop. v1: - Link: https://lore.kernel.org/linux-hardening/ZiwqqZCa7PK9bzCX@neat/ include/net/bluetooth/hci.h | 8 ++-- net/bluetooth/hci_event.c | 3 +- net/bluetooth/hci_sync.c | 84 +++++++++++++++---------------------- 3 files changed, 40 insertions(+), 55 deletions(-)