Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4158-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Apr 2024 11:16:11 -0700
From: Kees Cook <keescook@chromium.org>
To: "Gustavo A. R. Silva" <gustavoars@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>,
	Johan Hedberg <johan.hedberg@gmail.com>,
	Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
	"David S. Miller" <davem@davemloft.net>,
	Eric Dumazet <edumazet@google.com>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	linux-bluetooth@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH v2][next] Bluetooth: hci_conn, hci_sync: Use
 __counted_by() in multiple structs and avoid -Wfamnae warnings
Message-ID: <202404291110.6159F7EA5@keescook>
References: <ZiwwPmCvU25YzWek@neat>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ZiwwPmCvU25YzWek@neat>

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...


-- 
Kees Cook