[PATCH] tty: rfcomm: prefer struct_size over open coded arithmetic

[PATCH] tty: rfcomm: 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 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(-)

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;
-- 
2.25.1

Re: [PATCH] tty: rfcomm: prefer struct_size over open coded arithmetic

[Kees Cook]

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

-- 
Kees Cook