| Message ID | AS8PR02MB7237ECD397BDB7F529ADC7468BE12@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Bluetooth: hci_core: Prefer struct_size over open coded arithmetic | expand |
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>
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
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
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;
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(-)