| Message ID | AS8PR02MB723725E0069F7AE8F64E876E8B142@AS8PR02MB7237.eurprd02.prod.outlook.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tty: rfcomm: prefer struct_size over open coded arithmetic | expand |
On Sun, Apr 28, 2024 at 03:29:34PM +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 { u16 dev_num; > struct rfcomm_dev_info dev_info[]; > }; Similar to before, this should gain __counted_by(), and the logic using dev_info[] refactored slightly to gain coverage. > > 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 remove the "size" variable as it is no longer needed. > This way, the code is more readable 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> > --- > Hi, > > The Coccinelle script used to detect this code pattern is the following: > > virtual report > > @rule1@ > type t1; > type t2; > identifier i0; > identifier i1; > identifier i2; > identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; > position p1; > @@ > > i0 = sizeof(t1) + sizeof(t2) * i1; > ... > i2 = ALLOC@p1(..., i0, ...); > > @script:python depends on report@ > p1 << rule1.p1; > @@ > > msg = "WARNING: verify allocation on line %s" % (p1[0].line) > coccilib.report.print_report(p1[0],msg) > > Regards, > Erick > --- > net/bluetooth/rfcomm/tty.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c > index 69c75c041fe1..bdc64c8fb85b 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,9 +515,7 @@ 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); Luckily, "size" can't overflow. Max value seems to be around 1834980, but I'd rather this be in struct_size() as you have it below. Good! > - > - dl = kzalloc(size, GFP_KERNEL); > + dl = kzalloc(struct_size(dl, dev_info, dev_num), GFP_KERNEL); > if (!dl) > return -ENOMEM; When you add __counted_by, this will need to be added here: dl->dev_num = dev_num; Continuing... di = dl->dev_info; ... 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); tty_port_put(&dev->port); if (++n >= dev_num) break; } Hmpf. I'd rather this code use di[n] instead of (di + n) -- that's much more idiomatic. > @@ -542,9 +540,7 @@ static int rfcomm_get_dev_list(void __user *arg) > mutex_unlock(&rfcomm_dev_lock); > > dl->dev_num = n; And this reset of dl->dev_num can stay as-is, since it's reducing the number of valid entries, in can &rfcomm_dev_list is smaller than the dev_num count userspace offered. > - 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); > Otherwise looks good! -Kees
diff --git a/net/bluetooth/rfcomm/tty.c b/net/bluetooth/rfcomm/tty.c index 69c75c041fe1..bdc64c8fb85b 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,9 +515,7 @@ 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; @@ -542,9 +540,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 remove the "size" variable as it is no longer needed. This way, the code is more readable 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> --- Hi, The Coccinelle script used to detect this code pattern is the following: virtual report @rule1@ type t1; type t2; identifier i0; identifier i1; identifier i2; identifier ALLOC =~ "kmalloc|kzalloc|kmalloc_node|kzalloc_node|vmalloc|vzalloc|kvmalloc|kvzalloc"; position p1; @@ i0 = sizeof(t1) + sizeof(t2) * i1; ... i2 = ALLOC@p1(..., i0, ...); @script:python depends on report@ p1 << rule1.p1; @@ msg = "WARNING: verify allocation on line %s" % (p1[0].line) coccilib.report.print_report(p1[0],msg) Regards, Erick --- net/bluetooth/rfcomm/tty.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)