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.arc...@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
