| Message ID | AS8PR02MB7237262C62B054FABD7229168BE12@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2] tty: rfcomm: prefer struct_size over open coded arithmetic | expand |
On Sun, May 12, 2024 at 01:17:24PM +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 rfcomm_dev_list_req" and > this structure ends in a flexible array: > > struct rfcomm_dev_list_req { > [...] > struct rfcomm_dev_info dev_info[]; > }; > > 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 di[n] instead of (di + 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 good! Reviewed-by: Kees Cook <keescook@chromium.org> > [...] > - bacpy(&(di + n)->src, &dev->src); > - bacpy(&(di + n)->dst, &dev->dst); > + bacpy(&di[n].src, &dev->src); > + bacpy(&di[n].dst, &dev->dst); Not an issue with your patch, but this helper is really pointless in the Bluetooth tree: static inline void bacpy(bdaddr_t *dst, const bdaddr_t *src) { memcpy(dst, src, sizeof(bdaddr_t)); } So the above could just be: di[n].src = dev->src; di[n].dst = dev->dst; :P -Kees
On 12. 05. 24, 13:17, 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 rfcomm_dev_list_req" and > this structure ends in a flexible array: ... > --- a/include/net/bluetooth/rfcomm.h > +++ b/include/net/bluetooth/rfcomm.h ... > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg) > list_for_each_entry(dev, &rfcomm_dev_list, list) { > if (!tty_port_get(&dev->port)) > continue; > - (di + n)->id = dev->id; > - (di + n)->flags = dev->flags; > - (di + n)->state = dev->dlc->state; > - (di + n)->channel = dev->channel; > - bacpy(&(di + n)->src, &dev->src); > - bacpy(&(di + n)->dst, &dev->dst); > + di[n].id = dev->id; > + di[n].flags = dev->flags; > + di[n].state = dev->dlc->state; > + di[n].channel = dev->channel; > + bacpy(&di[n].src, &dev->src); > + bacpy(&di[n].dst, &dev->dst); This does not relate much to "prefer struct_size over open coded arithmetic". It should have been in a separate patch. Other than that, LGTM. thanks,
Hi Jiri, Eric, On Mon, May 13, 2024 at 1:07 AM Jiri Slaby <jirislaby@kernel.org> wrote: > > On 12. 05. 24, 13:17, 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 rfcomm_dev_list_req" and > > this structure ends in a flexible array: > ... > > --- a/include/net/bluetooth/rfcomm.h > > +++ b/include/net/bluetooth/rfcomm.h > ... > > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg) > > list_for_each_entry(dev, &rfcomm_dev_list, list) { > > if (!tty_port_get(&dev->port)) > > continue; > > - (di + n)->id = dev->id; > > - (di + n)->flags = dev->flags; > > - (di + n)->state = dev->dlc->state; > > - (di + n)->channel = dev->channel; > > - bacpy(&(di + n)->src, &dev->src); > > - bacpy(&(di + n)->dst, &dev->dst); > > + di[n].id = dev->id; > > + di[n].flags = dev->flags; > > + di[n].state = dev->dlc->state; > > + di[n].channel = dev->channel; > > + bacpy(&di[n].src, &dev->src); > > + bacpy(&di[n].dst, &dev->dst); > > This does not relate much to "prefer struct_size over open coded > arithmetic". It should have been in a separate patch. +1, please split these changes into its own patch so we can apply it separately. > Other than that, LGTM. > > thanks, > -- > js > suse labs >
Hi Kees, Jiri and Luiz, First of all, thanks for the reviews. On Mon, May 13, 2024 at 12:29:04PM -0400, Luiz Augusto von Dentz wrote: > Hi Jiri, Eric, > > On Mon, May 13, 2024 at 1:07 AM Jiri Slaby <jirislaby@kernel.org> wrote: > > > > On 12. 05. 24, 13:17, 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 rfcomm_dev_list_req" and > > > this structure ends in a flexible array: > > ... > > > --- a/include/net/bluetooth/rfcomm.h > > > +++ b/include/net/bluetooth/rfcomm.h > > ... > > > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg) > > > list_for_each_entry(dev, &rfcomm_dev_list, list) { > > > if (!tty_port_get(&dev->port)) > > > continue; > > > - (di + n)->id = dev->id; > > > - (di + n)->flags = dev->flags; > > > - (di + n)->state = dev->dlc->state; > > > - (di + n)->channel = dev->channel; > > > - bacpy(&(di + n)->src, &dev->src); > > > - bacpy(&(di + n)->dst, &dev->dst); > > > + di[n].id = dev->id; > > > + di[n].flags = dev->flags; > > > + di[n].state = dev->dlc->state; > > > + di[n].channel = dev->channel; > > > + bacpy(&di[n].src, &dev->src); > > > + bacpy(&di[n].dst, &dev->dst); > > > > This does not relate much to "prefer struct_size over open coded > > arithmetic". It should have been in a separate patch. > > +1, please split these changes into its own patch so we can apply it separately. Ok, no problem. Also, I will simplify the "bacpy" lines with direct assignments as Kees suggested: di[n].src = dev->src; di[n].dst = dev->dst; instead of: bacpy(&di[n].src, &dev->src); bacpy(&di[n].dst, &dev->dst); Regards, Erick > > Other than that, LGTM. > > > > thanks, > > -- > > js > > suse labs > > > > > -- > Luiz Augusto von Dentz
On Mon, May 13, 2024 at 07:12:57PM +0200, Erick Archer wrote: > Hi Kees, Jiri and Luiz, > First of all, thanks for the reviews. > > On Mon, May 13, 2024 at 12:29:04PM -0400, Luiz Augusto von Dentz wrote: > > Hi Jiri, Eric, > > > > On Mon, May 13, 2024 at 1:07 AM Jiri Slaby <jirislaby@kernel.org> wrote: > > > > > > On 12. 05. 24, 13:17, 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 rfcomm_dev_list_req" and > > > > this structure ends in a flexible array: > > > ... > > > > --- a/include/net/bluetooth/rfcomm.h > > > > +++ b/include/net/bluetooth/rfcomm.h > > > ... > > > > @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg) > > > > list_for_each_entry(dev, &rfcomm_dev_list, list) { > > > > if (!tty_port_get(&dev->port)) > > > > continue; > > > > - (di + n)->id = dev->id; > > > > - (di + n)->flags = dev->flags; > > > > - (di + n)->state = dev->dlc->state; > > > > - (di + n)->channel = dev->channel; > > > > - bacpy(&(di + n)->src, &dev->src); > > > > - bacpy(&(di + n)->dst, &dev->dst); > > > > + di[n].id = dev->id; > > > > + di[n].flags = dev->flags; > > > > + di[n].state = dev->dlc->state; > > > > + di[n].channel = dev->channel; > > > > + bacpy(&di[n].src, &dev->src); > > > > + bacpy(&di[n].dst, &dev->dst); > > > > > > This does not relate much to "prefer struct_size over open coded > > > arithmetic". It should have been in a separate patch. > > > > +1, please split these changes into its own patch so we can apply it separately. > > Ok, no problem. Also, I will simplify the "bacpy" lines with direct > assignments as Kees suggested: > > di[n].src = dev->src; > di[n].dst = dev->dst; > > instead of: > > bacpy(&di[n].src, &dev->src); > bacpy(&di[n].dst, &dev->dst); I think that's a separate thing and you can leave bacpy() as-is for now.
diff --git a/include/net/bluetooth/rfcomm.h b/include/net/bluetooth/rfcomm.h index 99d26879b02a..c05882476900 100644 --- a/include/net/bluetooth/rfcomm.h +++ b/include/net/bluetooth/rfcomm.h @@ -355,7 +355,7 @@ struct rfcomm_dev_info { struct rfcomm_dev_list_req { u16 dev_num; - struct rfcomm_dev_info dev_info[]; + struct rfcomm_dev_info dev_info[] __counted_by(dev_num); }; int rfcomm_dev_ioctl(struct sock *sk, unsigned int cmd, void __user *arg); diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c index 69c75c041fe1..af80d599c337 100644 --- a/net/bluetooth/rfcomm/tty.c +++ b/net/bluetooth/rfcomm/tty.c @@ -504,7 +504,7 @@ static int rfcomm_get_dev_list(void __user *arg) struct rfcomm_dev *dev; struct rfcomm_dev_list_req *dl; struct rfcomm_dev_info *di; - int n = 0, size, err; + int n = 0, err; u16 dev_num; BT_DBG(""); @@ -515,12 +515,11 @@ static int rfcomm_get_dev_list(void __user *arg) if (!dev_num || dev_num > (PAGE_SIZE * 4) / sizeof(*di)) return -EINVAL; - size = sizeof(*dl) + dev_num * sizeof(*di); - - dl = kzalloc(size, GFP_KERNEL); + dl = kzalloc(struct_size(dl, dev_info, dev_num), GFP_KERNEL); if (!dl) return -ENOMEM; + dl->dev_num = dev_num; di = dl->dev_info; mutex_lock(&rfcomm_dev_lock); @@ -528,12 +527,12 @@ static int rfcomm_get_dev_list(void __user *arg) list_for_each_entry(dev, &rfcomm_dev_list, list) { if (!tty_port_get(&dev->port)) continue; - (di + n)->id = dev->id; - (di + n)->flags = dev->flags; - (di + n)->state = dev->dlc->state; - (di + n)->channel = dev->channel; - bacpy(&(di + n)->src, &dev->src); - bacpy(&(di + n)->dst, &dev->dst); + di[n].id = dev->id; + di[n].flags = dev->flags; + di[n].state = dev->dlc->state; + di[n].channel = dev->channel; + bacpy(&di[n].src, &dev->src); + bacpy(&di[n].dst, &dev->dst); tty_port_put(&dev->port); if (++n >= dev_num) break; @@ -542,9 +541,7 @@ static int rfcomm_get_dev_list(void __user *arg) mutex_unlock(&rfcomm_dev_lock); dl->dev_num = n; - size = sizeof(*dl) + n * sizeof(*di); - - err = copy_to_user(arg, dl, size); + err = copy_to_user(arg, dl, struct_size(dl, dev_info, 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 rfcomm_dev_list_req" and this structure ends in a flexible array: struct rfcomm_dev_list_req { [...] struct rfcomm_dev_info dev_info[]; }; 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 di[n] instead of (di + 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> --- Changes in v2: - Add the __counted_by() attribute (Kees Cook). - Refactor the list_for_each_entry() loop to use di[n] instead of (di + n) (Kees Cook). Previous versions: v1 -> https://lore.kernel.org/linux-hardening/AS8PR02MB723725E0069F7AE8F64E876E8B142@AS8PR02MB7237.eurprd02.prod.outlook.com/ --- include/net/bluetooth/rfcomm.h | 2 +- net/bluetooth/rfcomm/tty.c | 23 ++++++++++------------- 2 files changed, 11 insertions(+), 14 deletions(-)