Re: [PATCH] perf/ring_buffer: Prefer struct_size over open coded arithmetic

[Kees Cook <keescook@chromium.org>]

On Mon, Apr 29, 2024 at 07:40:58PM +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 "rb" variable is a pointer to "struct perf_buffer" and this
> structure ends in a flexible array:
> 
> struct perf_buffer {
> 	[...]
        int                             nr_pages;       /* nr of data pages  */
	...
> 	void	*data_pages[];
> };

This should gain __counted_by(nr_pages) with a little refactoring.

> 
> 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_node() functions.
> 
> In the "rb_alloc" function defined in the else branch of the macro
> 
>  #ifndef CONFIG_PERF_USE_VMALLOC
> 
> the count in the struct_size helper is the literal "1" since only one
> pointer to void is allocated. Also, 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)
> ...
> i0 += 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
> ---
>  kernel/events/ring_buffer.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
> index 4013408ce012..e68b02a56382 100644
> --- a/kernel/events/ring_buffer.c
> +++ b/kernel/events/ring_buffer.c
> @@ -822,9 +822,7 @@ struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
>  	unsigned long size;
>  	int i, node;
>  
> -	size = sizeof(struct perf_buffer);
> -	size += nr_pages * sizeof(void *);
> -
> +	size = struct_size(rb, data_pages, nr_pages);
>  	if (order_base_2(size) > PAGE_SHIFT+MAX_PAGE_ORDER)
>  		goto fail;

This looks good. Continuing...

        node = (cpu == -1) ? cpu : cpu_to_node(cpu);
        rb = kzalloc_node(size, GFP_KERNEL, node);
        if (!rb)
                goto fail;

then move this line up from below the array-writing loop:

        rb->nr_pages = nr_pages;


>  
> @@ -916,15 +914,11 @@ void rb_free(struct perf_buffer *rb)
>  struct perf_buffer *rb_alloc(int nr_pages, long watermark, int cpu, int flags)
>  {
>  	struct perf_buffer *rb;
> -	unsigned long size;
>  	void *all_buf;
>  	int node;
>  
> -	size = sizeof(struct perf_buffer);
> -	size += sizeof(void *);
> -
>  	node = (cpu == -1) ? cpu : cpu_to_node(cpu);
> -	rb = kzalloc_node(size, GFP_KERNEL, node);
> +	rb = kzalloc_node(struct_size(rb, data_pages, 1), GFP_KERNEL, node);
>  	if (!rb)
>  		goto fail;

Same move here:

        rb->nr_pages = nr_pages;

Otherwise, yes, looks good!

-Kees

-- 
Kees Cook