[PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic

[PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic

[Erick Archer]

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

Re: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic

[Kees Cook]

On Sun, May 12, 2024 at 04:17:06PM +0200, Erick Archer wrote:
> 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>

Looks right to me. Thanks!

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic

[Luiz Augusto von Dentz]

Hi Eric,

On Sun, May 12, 2024 at 11:08 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, May 12, 2024 at 04:17:06PM +0200, Erick Archer wrote:
> > 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).

Have the change above split on its own patch.

> > 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>
>
> Looks right to me. Thanks!
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> --
> Kees Cook



-- 
Luiz Augusto von Dentz

Re: [PATCH] Bluetooth: hci_core: Prefer struct_size over open coded arithmetic

[Erick Archer]

Hi Kees and Luiz,
First of all, thanks for the reviews.

On Mon, May 13, 2024 at 12:31:29PM -0400, Luiz Augusto von Dentz wrote:
> Hi Eric,
> 
> On Sun, May 12, 2024 at 11:08 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Sun, May 12, 2024 at 04:17:06PM +0200, Erick Archer wrote:
> > > [...]
> > > 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).
> 
> Have the change above split on its own patch.

Ok, no problem. I will send a new version.

Regards,
Erick