Received-SPF: pass (google.com: domain of linux-bluetooth+bounces-4546-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: Erick Archer <erick.archer@outlook.com>
To: 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>,
	Kees Cook <keescook@chromium.org>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	Nathan Chancellor <nathan@kernel.org>,
	Nick Desaulniers <ndesaulniers@google.com>,
	Bill Wendling <morbo@google.com>,
	Justin Stitt <justinstitt@google.com>
Cc: Erick Archer <erick.archer@outlook.com>,
	linux-bluetooth@vger.kernel.org,
	netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	linux-hardening@vger.kernel.org,
	llvm@lists.linux.dev
Subject: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic
Date: Sun, 12 May 2024 16:17:06 +0200
Message-ID:
 <AS8PR02MB7237ECD397BDB7F529ADC7468BE12@AS8PR02MB7237.eurprd02.prod.outlook.com>
Content-Transfer-Encoding: 8bit
Content-Type: text/plain
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-MessageSentRepresentingType: 1
X-MS-PublicTrafficType: Email
X-MS-TrafficTypeDiagnostic: AS8PR02MB7237:EE_|AS4PR02MB8599:EE_
X-MS-Office365-Filtering-Correlation-Id: 0a4dc55f-1fcd-4cbc-02f2-08dc728e3dbf
X-Microsoft-Antispam:
	BCL:0;ARA:14566002|461199019|1602099003|440099019|3412199016|1710799017;
X-Microsoft-Antispam-Message-Info:
	BTZrkYT1OZkUW06Vd+hj0UW8mywzh+A1RcNelQlp2wkVnHkNQZZq5G8cgboxiwM7V8a+NpaTruTYsGhpVB1S11amWOl/XCFShF//Ug/mUHRqmYvSxzUT0iT9+/PmHiir7Twm89F4qu+CNGwfsUip5vorzT0ZFqoWVpAvKGHHundPqnxmKmtW/Y5+/eN/Ft5LGsOh2Dk6SmrJRxF5E9dtHQP/TSKTSorsfbHHasq1vHKyP7PNADzNBY6Lhp5dk3IaTwzRoVm6S9J+wePtoNAzIaUmou+jm8wBG1rBVZs8/iNcnXYrBW4j7g/VlZg+m4CzojHZ1gKATcsMMfEuD6jMIsHzzW7rWN/qPXAXZCz36fK7kpF3ed2r28k8objGJNjwY6p5HLzrUIKgRGySeZFWwk0LCKINA2sVSsbo1tuBREUyx5Er8FF2LCTKyI4TBn4IIVsgLjq+1vs6R8zmwHXbxROrvb3kD4naP9zLi2rRYWwhj4Ftwp4hxTm9oYLQXXFGM47Xi2pRYFJkG34CmdSW7dWQxVdwmUATOpNhBdlxkstNtuEM1yY5yqsqFcMpM3nd7lXVJx1EzoXepYktSivwl2hVD+LjMxfUpsZKrb0SSv4q0b8B0FfRIrAnk9UkXkkJAD0W2oi7tarlUV2IhfBdNSh73vYdN7PQSq2Hveu7dKlsIghfFumHYIm9wd3xiqYp6N0e9MS9YxBJHpGsfrZIhg==
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?us-ascii?Q?suOZYN5I6JW2Q2xl6hl5JgOlrABrdjIrjruA9IFJHnogcytQiV9gPWVhMkbR?=
 =?us-ascii?Q?tAN7zG8fGsevoXLytC2OmIg86ltNJj52tKCG6tOTVAGJGql+ijHjeMb1S/Xa?=
 =?us-ascii?Q?0APbHiZ7dom8UftNMEWXAD7KPpyVNpe22vM2DaJ+SiLpzshx1E44/jSNdzzF?=
 =?us-ascii?Q?MegcCzdfL5D902UI5xUW7az0qLXymPMo/NKwOP0OREkjssUSKriSbNjvWibV?=
 =?us-ascii?Q?+IWzCLuR9o521xkMNYeahTF4dwoHKRj35CV9N2t6epaFcgYUqValnRchC3tD?=
 =?us-ascii?Q?6Yptfvup1J5x01AYfGLBlET48zpbgSy3uiFOPZty+hjSMUgQYWJKjzkFmpC8?=
 =?us-ascii?Q?NLlBcJLMygN/ncMzfPP2skgEwQUmF0X9R9msW5YG+rga1mqBEL5/qj9LwEhH?=
 =?us-ascii?Q?KmFq7Ce/eICGOkyF9U9Cv8bCABueaahdC/zZA7NSpIPL49Hj4waFz48qOQeV?=
 =?us-ascii?Q?YRHHvzCdgPYOA1E9qsJ9R28WuWDz9kQO/0aRg1hvEvhpxsgw0sdHRNYzMbAf?=
 =?us-ascii?Q?r7B8olqa/j021TludxeRCDLeAX+PjexDXCAawWHUuApPPOfvd+vckq0jJ0JK?=
 =?us-ascii?Q?XjZWKNVM2/UeZy8Vs1PirXkwthHdOEB9SY2KbVQzXLe8DWtOUCJtZQcGnt92?=
 =?us-ascii?Q?1y4PTgvqsPdxE9/91s8nhIdBKMajcvQyxwewETcokKJKaHBW504Opa77r6WX?=
 =?us-ascii?Q?F6XenwW2pOSY5erko4ClU73b5o4w6+5M6rFlyZxb9DfYjm56rkoLE60CmfEA?=
 =?us-ascii?Q?+bFKAdIIL2DbKs0t7vldf2kub0P49kp30My924mIxGBW32HpDxRrAi7aejhx?=
 =?us-ascii?Q?/phM/psITLTDGd82U9rHPvcer1wosEqN3dHk99l0ixLQgQPYGviL9BdXnj9Q?=
 =?us-ascii?Q?nqcELWZVH3QEF3ypBvRQpdC+RRhn3ZlK2jcPtqBnt8a67OzLRmYkthhEh9V9?=
 =?us-ascii?Q?Y+ALVJEVlc1tnc7pbtChxatHaaTjmbLhX90ZI6UfW0jXxUa21f3ivxBwKWgt?=
 =?us-ascii?Q?Hhf/VCcDzlXEpu5rNAornuHEczfx6Z2BGqwP6eH7DBMB809To4+CakIqkTdr?=
 =?us-ascii?Q?h8DfusI4ldn2amlBvB9I4e+WHshLZbvr/6mLbjBhjb9J/Yxay0/JVwu2CKCg?=
 =?us-ascii?Q?uD4NZBqxTBCVOZeaOmqxUVaFrg+GO6VSQUeeCk/ldOgfpIItL4xBZQSNs8gk?=
 =?us-ascii?Q?TKE7QpNlekfk8A6qNOBv/sRF88ZyERGTcsBY+Gv/sGyWDzZSGXhVCHWyKyru?=
 =?us-ascii?Q?1F2b3VjToLF72MsC4Uhm?=
X-OriginatorOrg: outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a4dc55f-1fcd-4cbc-02f2-08dc728e3dbf
X-MS-Exchange-CrossTenant-AuthSource: AS8PR02MB7237.eurprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 May 2024 14:17:22.9481
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa
X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg:
	00000000-0000-0000-0000-000000000000
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS4PR02MB8599

This is an effort to get rid of all multiplications from allocation
functions in order to prevent integer overflows [1][2].

As the "dl" variable is a pointer to "struct hci_dev_list_req" and this
structure ends in a flexible array:

struct hci_dev_list_req {
	[...]
	struct hci_dev_req dev_req[];	/* hci_dev_req structures */
};

the preferred way in the kernel is to use the struct_size() helper to
do the arithmetic instead of the calculation "size + count * size" in
the kzalloc() and copy_to_user() functions.

At the same time, 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).

In this case, it is important to note that the logic needs a little
refactoring to ensure that the "dev_num" member is initialized before
the first access to the flex array. Specifically, add the assignment
before the list_for_each_entry() loop.

Also remove the "size" variable as it is no longer needed and refactor
the list_for_each_entry() loop to use dr[n] instead of (dr + n).

This way, the code is more readable, idiomatic and safer.

This code was detected with the help of Coccinelle, and audited and
modified manually.

Link: https://www.kernel.org/doc/html/latest/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [1]
Link: https://github.com/KSPP/linux/issues/160 [2]

Signed-off-by: Erick Archer <erick.archer@outlook.com>
---
 include/net/bluetooth/hci_sock.h |  2 +-
 net/bluetooth/hci_core.c         | 15 ++++++---------
 2 files changed, 7 insertions(+), 10 deletions(-)

diff --git a/include/net/bluetooth/hci_sock.h b/include/net/bluetooth/hci_sock.h
index 9949870f7d78..13e8cd4414a1 100644
--- a/include/net/bluetooth/hci_sock.h
+++ b/include/net/bluetooth/hci_sock.h
@@ -144,7 +144,7 @@ struct hci_dev_req {
 
 struct hci_dev_list_req {
 	__u16  dev_num;
-	struct hci_dev_req dev_req[];	/* hci_dev_req structures */
+	struct hci_dev_req dev_req[] __counted_by(dev_num);
 };
 
 struct hci_conn_list_req {
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index adfd53a9fcd4..cae8a67bcd62 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -837,7 +837,7 @@ int hci_get_dev_list(void __user *arg)
 	struct hci_dev *hdev;
 	struct hci_dev_list_req *dl;
 	struct hci_dev_req *dr;
-	int n = 0, size, err;
+	int n = 0, err;
 	__u16 dev_num;
 
 	if (get_user(dev_num, (__u16 __user *) arg))
@@ -846,12 +846,11 @@ int hci_get_dev_list(void __user *arg)
 	if (!dev_num || dev_num > (PAGE_SIZE * 2) / sizeof(*dr))
 		return -EINVAL;
 
-	size = sizeof(*dl) + dev_num * sizeof(*dr);
-
-	dl = kzalloc(size, GFP_KERNEL);
+	dl = kzalloc(struct_size(dl, dev_req, dev_num), GFP_KERNEL);
 	if (!dl)
 		return -ENOMEM;
 
+	dl->dev_num = dev_num;
 	dr = dl->dev_req;
 
 	read_lock(&hci_dev_list_lock);
@@ -865,8 +864,8 @@ int hci_get_dev_list(void __user *arg)
 		if (hci_dev_test_flag(hdev, HCI_AUTO_OFF))
 			flags &= ~BIT(HCI_UP);
 
-		(dr + n)->dev_id  = hdev->id;
-		(dr + n)->dev_opt = flags;
+		dr[n].dev_id  = hdev->id;
+		dr[n].dev_opt = flags;
 
 		if (++n >= dev_num)
 			break;
@@ -874,9 +873,7 @@ int hci_get_dev_list(void __user *arg)
 	read_unlock(&hci_dev_list_lock);
 
 	dl->dev_num = n;
-	size = sizeof(*dl) + n * sizeof(*dr);
-
-	err = copy_to_user(arg, dl, size);
+	err = copy_to_user(arg, dl, struct_size(dl, dev_req, n));
 	kfree(dl);
 
 	return err ? -EFAULT : 0;
-- 
2.25.1