[PATCH] Bluetooth: hci_conn: Refactor hci_bind_bis() since it always succeeds

[PATCH] Bluetooth: hci_conn: Refactor hci_bind_bis() since it always succeeds

[Kees Cook]

The compiler thinks "conn" might be NULL after a call to hci_bind_bis(),
which cannot happen. Avoid any confusion by just making it not return a
value since it cannot fail. Fixes the warnings seen with GCC 13:

In function 'arch_atomic_dec_and_test',
    inlined from 'atomic_dec_and_test' at ../include/linux/atomic/atomic-instrumented.h:576:9,
    inlined from 'hci_conn_drop' at ../include/net/bluetooth/hci_core.h:1391:6,
    inlined from 'hci_connect_bis' at ../net/bluetooth/hci_conn.c:2124:3:
../arch/x86/include/asm/rmwcc.h:37:9: warning: array subscript 0 is outside array bounds of 'atomic_t[0]' [-Warray-bounds=]
   37 |         asm volatile (fullop CC_SET(cc) \
      |         ^~~
...
In function 'hci_connect_bis':
cc1: note: source object is likely at address zero

Fixes: eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections")
Cc: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: linux-bluetooth@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 net/bluetooth/hci_conn.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index acf563fbdfd9..61a34801e61e 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -1981,16 +1981,14 @@ static void hci_iso_qos_setup(struct hci_dev *hdev, struct hci_conn *conn,
 		qos->latency = conn->le_conn_latency;
 }
 
-static struct hci_conn *hci_bind_bis(struct hci_conn *conn,
-				     struct bt_iso_qos *qos)
+static void hci_bind_bis(struct hci_conn *conn,
+			 struct bt_iso_qos *qos)
 {
 	/* Update LINK PHYs according to QoS preference */
 	conn->le_tx_phy = qos->out.phy;
 	conn->le_tx_phy = qos->out.phy;
 	conn->iso_qos = *qos;
 	conn->state = BT_BOUND;
-
-	return conn;
 }
 
 static int create_big_sync(struct hci_dev *hdev, void *data)
@@ -2119,11 +2117,7 @@ struct hci_conn *hci_connect_bis(struct hci_dev *hdev, bdaddr_t *dst,
 	if (IS_ERR(conn))
 		return conn;
 
-	conn = hci_bind_bis(conn, qos);
-	if (!conn) {
-		hci_conn_drop(conn);
-		return ERR_PTR(-ENOMEM);
-	}
+	hci_bind_bis(conn, qos);
 
 	/* Add Basic Announcement into Peridic Adv Data if BASE is set */
 	if (base_len && base) {
-- 
2.34.1

Re: [PATCH] Bluetooth: hci_conn: Refactor hci_bind_bis() since it always succeeds

[Simon Horman]

On Fri, Jan 27, 2023 at 04:51:54PM -0800, Kees Cook wrote:
> The compiler thinks "conn" might be NULL after a call to hci_bind_bis(),
> which cannot happen. Avoid any confusion by just making it not return a
> value since it cannot fail. Fixes the warnings seen with GCC 13:
> 
> In function 'arch_atomic_dec_and_test',
>     inlined from 'atomic_dec_and_test' at ../include/linux/atomic/atomic-instrumented.h:576:9,
>     inlined from 'hci_conn_drop' at ../include/net/bluetooth/hci_core.h:1391:6,
>     inlined from 'hci_connect_bis' at ../net/bluetooth/hci_conn.c:2124:3:
> ../arch/x86/include/asm/rmwcc.h:37:9: warning: array subscript 0 is outside array bounds of 'atomic_t[0]' [-Warray-bounds=]
>    37 |         asm volatile (fullop CC_SET(cc) \
>       |         ^~~
> ...
> In function 'hci_connect_bis':
> cc1: note: source object is likely at address zero
> 
> Fixes: eca0ae4aea66 ("Bluetooth: Add initial implementation of BIS connections")
...
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  net/bluetooth/hci_conn.c | 12 +++---------
>  1 file changed, 3 insertions(+), 9 deletions(-)

Is this really a 'fix' ?

In any case, the change looks good to me.

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH] Bluetooth: hci_conn: Refactor hci_bind_bis() since it always succeeds

[patchwork-bot+bluetooth]

Hello:

This patch was applied to bluetooth/bluetooth-next.git (master)
by Luiz Augusto von Dentz <luiz.von.dentz@intel.com>:

On Fri, 27 Jan 2023 16:51:54 -0800 you wrote:
> The compiler thinks "conn" might be NULL after a call to hci_bind_bis(),
> which cannot happen. Avoid any confusion by just making it not return a
> value since it cannot fail. Fixes the warnings seen with GCC 13:
> 
> In function 'arch_atomic_dec_and_test',
>     inlined from 'atomic_dec_and_test' at ../include/linux/atomic/atomic-instrumented.h:576:9,
>     inlined from 'hci_conn_drop' at ../include/net/bluetooth/hci_core.h:1391:6,
>     inlined from 'hci_connect_bis' at ../net/bluetooth/hci_conn.c:2124:3:
> ../arch/x86/include/asm/rmwcc.h:37:9: warning: array subscript 0 is outside array bounds of 'atomic_t[0]' [-Warray-bounds=]
>    37 |         asm volatile (fullop CC_SET(cc) \
>       |         ^~~
> ...
> In function 'hci_connect_bis':
> cc1: note: source object is likely at address zero
> 
> [...]

Here is the summary with links:
  - Bluetooth: hci_conn: Refactor hci_bind_bis() since it always succeeds
    https://git.kernel.org/bluetooth/bluetooth-next/c/d57d873e6851

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html