Received-SPF: pass (google.com: domain of linux-kernel+bounces-106973-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <38c7e956-9161-41d0-94a1-5f55481fd8a3@embeddedor.com>
Date: Mon, 18 Mar 2024 21:49:37 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] perf/x86/rapl: Prefer struct_size over open coded
 arithmetic
Content-Language: en-US
To: Kees Cook <keescook@chromium.org>, Erick Archer <erick.archer@gmx.com>
Cc: Peter Zijlstra <peterz@infradead.org>, Ingo Molnar <mingo@redhat.com>,
 Arnaldo Carvalho de Melo <acme@kernel.org>,
 Namhyung Kim <namhyung@kernel.org>, Mark Rutland <mark.rutland@arm.com>,
 Alexander Shishkin <alexander.shishkin@linux.intel.com>,
 Jiri Olsa <jolsa@kernel.org>, Ian Rogers <irogers@google.com>,
 Adrian Hunter <adrian.hunter@intel.com>, Thomas Gleixner
 <tglx@linutronix.de>, Borislav Petkov <bp@alien8.de>,
 Dave Hansen <dave.hansen@linux.intel.com>, "H. Peter Anvin" <hpa@zytor.com>,
 "Gustavo A. R. Silva" <gustavoars@kernel.org>, x86@kernel.org,
 linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-hardening@vger.kernel.org
References: <20240317164442.6729-1-erick.archer@gmx.com>
 <202403181635.46CBABD994@keescook>
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <202403181635.46CBABD994@keescook>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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@gmx.com>
> 
> Thanks!
> 
> Reviewed-by: Kees Cook <keescook@chromium.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