From nobody Sun May 19 10:14:45 2024 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C5EF7364A0; Fri, 26 Apr 2024 22:29:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714170540; cv=none; b=TnghySVUfe7BlOP8YQ9pEWdNVvUlpiQj7nBZYOE1JjybMPj7EW4teSWZfUnk2mSKT1Y5vhv78+eUVqSsYxvhlBUWStcXQJygSNuhuvUorVU2+nadprXW1tsJSYzhDoZD+21QZ/BijNP6espeB1YKDvPX3MyZ18QtjRP4AyZq7cM= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1714170540; c=relaxed/simple; bh=ELJHySFUSowC2UOJqdXHpOw3chABBbzFVeQLrdq5PXg=; h=Date:From:To:Cc:Subject:Message-ID:MIME-Version:Content-Type: Content-Disposition; b=ESTurIC2NpCbgDkpR+n2mUwySBDgfbzfHcVGS1aieyurNY1pyi418Ncl4SV3sSnoEWku6UA4v1Y32gURpSE1G9oXRYQV5ZIOCYwGOhFPSxOTGYPlM8aTxGo8xohjbxP/liBOR3CsZsbpZlv6qJxVO7pdntRfxTVR8JHOb8dYOvA= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=nC5mFa7Q; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="nC5mFa7Q" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4BF19C32781; Fri, 26 Apr 2024 22:28:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1714170540; bh=ELJHySFUSowC2UOJqdXHpOw3chABBbzFVeQLrdq5PXg=; h=Date:From:To:Cc:Subject:From; b=nC5mFa7Q5ogHzSx3A4Ap/Z5255P+7ZS93HjWyIudIq5bb6xlgr9+EEHC3SNfiZjlp Jt5A1azu8HAv+yRBi1GVDVTAXJI+1XZCCZDkomrpU5hzkllXpWvsipQXCL7z1CA1wy 5+gh8nq4LF5WI433zni9zKp0gHvooIZlwC7VXJv8QvF0k9s1QErDK2SSOqlPAMv+Rq UckxSsqeXZ7UtiU3+KZXMIZsoplfkrfww65/2R/suYSrb9Ty7xYK2OqUlvUqOs/Vhm LTFHGCx6TQi/ENLieFjYhKRlLM8Jlp9Ky5vhrfPh4XYtVhgNZmk3CEJcXJ+oHTC0c4 GRm00MsUxJG0A== Date: Fri, 26 Apr 2024 16:28:57 -0600 From: "Gustavo A. R. Silva" To: Marcel Holtmann , Johan Hedberg , Luiz Augusto von Dentz , "David S. Miller" , Eric Dumazet , Jakub Kicinski , Paolo Abeni Cc: linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, "Gustavo A. R. Silva" , linux-hardening@vger.kernel.org Subject: [PATCH][next] Bluetooth: hci_sync, hci_event: Use __counted_by() in multiple structs and avoid -Wfamnae warnings Message-ID: Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset="utf-8" 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 =3D 0; cp->num_cis; cp->num_cis--, i++) { ... 4314 handle =3D __le16_to_cpu(cp->cis[i].cis_handle); otherwise, only half (one iteration before `cp->num_cis =3D=3D 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. 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-no= t-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-no= t-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-no= t-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-no= t-at-end] Link: https://github.com/KSPP/linux/issues/202 Signed-off-by: Gustavo A. R. Silva --- include/net/bluetooth/hci.h | 8 ++-- net/bluetooth/hci_event.c | 3 +- net/bluetooth/hci_sync.c | 83 +++++++++++++++---------------------- 3 files changed, 39 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; =20 #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; =20 #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; =20 #define HCI_OP_LE_SET_PER_ADV_ENABLE 0x2040 @@ -2162,7 +2162,7 @@ struct hci_cis { =20 struct hci_cp_le_create_cis { __u8 num_cis; - struct hci_cis cis[]; + struct hci_cis cis[] __counted_by(num_cis); } __packed; =20 #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); =20 /* Remove connection if command failed */ - for (i =3D 0; cp->num_cis; cp->num_cis--, i++) { + for (i =3D 0; i < cp->num_cis; i++) { struct hci_conn *conn; u16 handle; =20 @@ -4323,6 +4323,7 @@ static void hci_cs_le_create_cis(struct hci_dev *hdev= , u8 status) hci_conn_del(conn); } } + cp->num_cis =3D 0; =20 if (pending) hci_le_create_cis_pending(hdev); diff --git a/net/bluetooth/hci_sync.c b/net/bluetooth/hci_sync.c index 9092b4d59545..c9e602d7b246 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) =20 static int hci_set_ext_scan_rsp_data_sync(struct hci_dev *hdev, u8 instanc= e) { - 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 =3D NULL; int err; =20 - memset(&pdu, 0, sizeof(pdu)); - if (instance) { adv =3D hci_find_adv_instance(hdev, instance); if (!adv || !adv->scan_rsp_changed) return 0; } =20 - len =3D eir_create_scan_rsp(hdev, instance, pdu.data); + len =3D eir_create_scan_rsp(hdev, instance, pdu->data); =20 - pdu.cp.handle =3D instance; - pdu.cp.length =3D len; - pdu.cp.operation =3D LE_SET_ADV_DATA_OP_COMPLETE; - pdu.cp.frag_pref =3D LE_SET_ADV_DATA_NO_FRAG; + pdu->handle =3D instance; + pdu->length =3D len; + pdu->operation =3D LE_SET_ADV_DATA_OP_COMPLETE; + pdu->frag_pref =3D LE_SET_ADV_DATA_NO_FRAG; =20 err =3D __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 =3D false; } else { - memcpy(hdev->scan_rsp_data, pdu.data, len); + memcpy(hdev->scan_rsp_data, pdu->data, len); hdev->scan_rsp_data_len =3D len; } =20 @@ -1411,14 +1407,10 @@ static int hci_set_per_adv_params_sync(struct hci_d= ev *hdev, u8 instance, =20 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; =20 - memset(&pdu, 0, sizeof(pdu)); - if (instance) { struct adv_info *adv =3D hci_find_adv_instance(hdev, instance); =20 @@ -1426,14 +1418,14 @@ static int hci_set_per_adv_data_sync(struct hci_dev= *hdev, u8 instance) return 0; } =20 - len =3D eir_create_per_adv_data(hdev, instance, pdu.data); + len =3D eir_create_per_adv_data(hdev, instance, pdu->data); =20 - pdu.cp.length =3D len; - pdu.cp.handle =3D instance; - pdu.cp.operation =3D LE_SET_ADV_DATA_OP_COMPLETE; + pdu->length =3D len; + pdu->handle =3D instance; + pdu->operation =3D LE_SET_ADV_DATA_OP_COMPLETE; =20 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); } =20 @@ -1727,31 +1719,27 @@ int hci_le_terminate_big_sync(struct hci_dev *hdev,= u8 handle, u8 reason) =20 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 =3D NULL; int err; =20 - memset(&pdu, 0, sizeof(pdu)); - if (instance) { adv =3D hci_find_adv_instance(hdev, instance); if (!adv || !adv->adv_data_changed) return 0; } =20 - len =3D eir_create_adv_data(hdev, instance, pdu.data); + len =3D eir_create_adv_data(hdev, instance, pdu->data); =20 - pdu.cp.length =3D len; - pdu.cp.handle =3D instance; - pdu.cp.operation =3D LE_SET_ADV_DATA_OP_COMPLETE; - pdu.cp.frag_pref =3D LE_SET_ADV_DATA_NO_FRAG; + pdu->length =3D len; + pdu->handle =3D instance; + pdu->operation =3D LE_SET_ADV_DATA_OP_COMPLETE; + pdu->frag_pref =3D LE_SET_ADV_DATA_NO_FRAG; =20 err =3D __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 =3D false; } else { - memcpy(hdev->adv_data, pdu.data, len); + memcpy(hdev->adv_data, pdu->data, len); hdev->adv_data_len =3D len; } =20 @@ -6496,10 +6484,8 @@ static int hci_le_create_conn_sync(struct hci_dev *h= dev, void *data) =20 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 =3D 0; struct hci_conn *conn; u8 cig =3D BT_ISO_QOS_CIG_UNSET; =20 @@ -6526,8 +6512,6 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) * remains pending. */ =20 - memset(&cmd, 0, sizeof(cmd)); - hci_dev_lock(hdev); =20 rcu_read_lock(); @@ -6564,7 +6548,7 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) goto done; =20 list_for_each_entry_rcu(conn, &hdev->conn_hash.list, list) { - struct hci_cis *cis =3D &cmd.cis[cmd.cp.num_cis]; + struct hci_cis *cis =3D &cmd->cis[aux_num_cis]; =20 if (hci_conn_check_create_cis(conn) || conn->iso_qos.ucast.cig !=3D cig) @@ -6573,9 +6557,9 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) set_bit(HCI_CONN_CREATE_CIS, &conn->flags); cis->acl_handle =3D cpu_to_le16(conn->parent->handle); cis->cis_handle =3D cpu_to_le16(conn->handle); - cmd.cp.num_cis++; + cmd->num_cis =3D ++aux_num_cis; =20 - if (cmd.cp.num_cis >=3D ARRAY_SIZE(cmd.cis)) + if (cmd->num_cis >=3D 0x1f) break; } =20 @@ -6584,14 +6568,13 @@ int hci_le_create_cis_sync(struct hci_dev *hdev) =20 hci_dev_unlock(hdev); =20 - if (!cmd.cp.num_cis) + if (!aux_num_cis) return 0; =20 /* 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); } =20 --=20 2.34.1