linux-kernel - Re: [PATCH] perf/x86/rapl: Prefer struct

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <38c7e956-9161-41d0-94a1-5f55481fd8a3@embeddedor.com>
Date: Mon, 18 Mar 2024 21:49:37 -0600
From: "Gustavo A. R. Silva" <gustavo@...eddedor.com>
To: Kees Cook <keescook@...omium.org>, Erick Archer <erick.archer@....com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
 Arnaldo Carvalho de Melo <acme@...nel.org>,
 Namhyung Kim <namhyung@...nel.org>, Mark Rutland <mark.rutland@....com>,
 Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
 Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
 Adrian Hunter <adrian.hunter@...el.com>, Thomas Gleixner
 <tglx@...utronix.de>, Borislav Petkov <bp@...en8.de>,
 Dave Hansen <dave.hansen@...ux.intel.com>, "H. Peter Anvin" <hpa@...or.com>,
 "Gustavo A. R. Silva" <gustavoars@...nel.org>, x86@...nel.org,
 linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-hardening@...r.kernel.org
Subject: Re: [PATCH] perf/x86/rapl: Prefer struct_size over open coded
 arithmetic

On 18/03/24 17:40, Kees Cook wrote:
> On Sun, Mar 17, 2024 at 05:44:42PM +0100, 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 "rapl_pmus" variable is a pointer to "struct rapl_pmus" and
>> this structure ends in a flexible array:
>>
>> struct rapl_pmus {
>> 	[...]
>> 	struct rapl_pmu *pmus[] __counted_by(maxdie);
>> };
>>
>> 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() function.
>>
>> This way, the code is more readable and safer.
>>
>> 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@....com>
> 
> Thanks!
> 
> Reviewed-by: Kees Cook <keescook@...omium.org>
> 
> I was inspired to come up with a Coccinelle script to find this pattern.
> This seems to do it, though it also removes the blank line. I'm not sure
> how to stop it from doing that. I'm running this treewide to see if I
> can find others...
> 
> // Options: --no-includes --include-headers

with --no-includes this one is missed in arch/x86/events/:

diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index 258e2cdf28fa..213fe48f9391 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -350,12 +350,11 @@ static void uncore_pmu_init_hrtimer(struct intel_uncore_box *box)
  static struct intel_uncore_box *uncore_alloc_box(struct intel_uncore_type *type,
                                                  int node)
  {
-       int i, size, numshared = type->num_shared_regs ;
+       int i, numshared = type->num_shared_regs ;
         struct intel_uncore_box *box;

-       size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);
-
-       box = kzalloc_node(size, GFP_KERNEL, node);
+       box = kzalloc_node(struct_size(box, shared_regs, numshared),
+                          GFP_KERNEL, node);
         if (!box)
                 return NULL;

Funny thing is that in this case it's quite convenient that the script removes
the blank like. :P

--
Gustavo