Re: [PATCH] checkpatch: add vmalloc() and vzalloc() to alloc with multiplies check

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4411 bytes --]

Hi Iurii, Kees,

> Date: 2026-10-14 20:20:06-0400
> From: Iurii Trukhin <yuri@trukhin.com>
>
> vmalloc() and vzalloc() have 2-factor multiplication argument forms
> vmalloc_array() and vcalloc() in <linux/vmalloc.h> that protect against
> integer overflow during size calculation.
> 
> Commit 070580b0b174 ("checkpatch: Suggest kmalloc_obj family for sizeof
> allocations") updated ALLOC_WITH_MULTIPLY for the (kv|k)[mz]alloc()
> family to suggest kmalloc_objs(type, count, gfp), while vmalloc_array()
> and vcalloc() retain the (count, size) signature.
> 
> Extend ALLOC_WITH_MULTIPLY to warn when vmalloc() or vzalloc() is called
> with a 2-factor sizeof() multiplication and suggest vmalloc_array() or
> vcalloc() respectively, including --fix support. Also include the 'v'
> prefix in ALLOC_ARRAY_ARGS so swapped sizeof() first arguments to
> vmalloc_array() and vcalloc() are flagged.
> 
> Link: https://github.com/KSPP/linux/issues/342
> Signed-off-by: Iurii Trukhin <yuri@trukhin.com>
> ---
>  scripts/checkpatch.pl | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 8a7787d228a6..651a46f1fc40 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -7355,10 +7355,11 @@ sub process {
>  		}
> 
> 
> -# check for (kv|k)[mz]alloc with multiplies that could be
> kmalloc_objs/kvmalloc_objs/kzalloc_objs/kvzalloc_objs
> +# check for (kv|k|v)[mz]alloc with multiplies that could be
> +# kmalloc_objs/kvmalloc_objs/kzalloc_objs/kvzalloc_objs/vmalloc_array/vcalloc
>  		if ($perl_version_ok &&
>  		    defined $stat &&
> -		    $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*,/)
> {
> +		    $stat =~
> /^\+\s*($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k|v)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)\s*(?:,|\))/)
> {
>  			my $oldfunc = $3;
>  			my $a1 = $4;
>  			my $a2 = $10;
> @@ -7366,6 +7367,8 @@ sub process {
>  			$newfunc = "kvmalloc_objs" if ($oldfunc eq "kvmalloc");
>  			$newfunc = "kvzalloc_objs" if ($oldfunc eq "kvzalloc");
>  			$newfunc = "kzalloc_objs" if ($oldfunc eq "kzalloc");
> +			$newfunc = "vmalloc_array" if ($oldfunc eq "vmalloc");
> +			$newfunc = "vcalloc" if ($oldfunc eq "vzalloc");
>  			my $r1 = $a1;
>  			my $r2 = $a2;
>  			if ($a1 =~ /^sizeof\s*\S/) {
> @@ -7381,9 +7384,13 @@ sub process {
>  					 "Prefer $newfunc over $oldfunc with multiply\n" . $herectx) &&
>  				    $cnt == 1 &&
>  				    $fix) {
> -					my $sized = trim($r2);
> -					$sized =~ s/^sizeof\s*\S\(?([^\)]*)\)?$/$1/;
> -					$fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . $sized . ', ' . trim($r1)/e;
> +					if ($oldfunc =~ /^v/) {
> +						$fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*(v[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . trim($r1) . ', ' . trim($r2)/e;
> +					} else {
> +						my $sized = trim($r2);
> +						$sized =~ s/^sizeof\s*\S\(?([^\)]*)\)?$/$1/;
> +						$fixed[$fixlinenr] =~
> s/\b($Lval)\s*\=\s*(?:$balanced_parens)?\s*((?:kv|k)[mz]alloc)\s*\(\s*($FuncArg)\s*\*\s*($FuncArg)/$1
> . ' = ' . "$newfunc(" . $sized . ', ' . trim($r1)/e;
> +					}

Could the suggested fix add a typeof() to the first parameter?  The only
reason (AFAIK) Kees avoided adding typeof() in the first parameter was
to minimize churn, but if someone is going to fix new code, it would be
better if they do it good.  The good thing about using types explicitly
is that it's safer.

That is, it should suggest
	kmalloc_objs(typeof(*p), count, gfp)
instead of
	kmalloc_objs(*p, count, gfp)


What do you think?


Have a lovely day!
Alex

>  				}
>  			}
>  		}
> @@ -7397,7 +7404,7 @@ sub process {
>  		}
> 
>  # check for alloc argument mismatch
> -		if ($line =~
> /\b((?:devm_)?((?:k|kv)?(calloc|malloc_array)(?:_node)?))\s*\(\s*sizeof\b/)
> {
> +		if ($line =~
> /\b((?:devm_)?((?:k|kv|v)?(calloc|malloc_array)(?:_node)?))\s*\(\s*sizeof\b/)
> {
>  			WARN("ALLOC_ARRAY_ARGS",
>  			     "$1 uses number as first arg, sizeof is generally wrong\n" .
> $herecurr);
>  		}
> -- 
> 2.56.0
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]