Messages in this thread | ![/](/images/icornerl.gif) | | Date | Mon, 29 Apr 2024 10:41:43 -0700 | From | Kees Cook <> | Subject | Re: [PATCH v3] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*() |
| |
On Sat, Apr 27, 2024 at 06:45:23PM +0200, Erick Archer wrote: > This is an effort to get rid of all multiplications from allocation > functions in order to prevent integer overflows [1]. > > Here the multiplication is obviously safe. However, using kcalloc*() > is more appropriate [2] and improves readability. This patch has no > effect on runtime behavior. > > Link: https://github.com/KSPP/linux/issues/162 [1] > Link: https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments [2] > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Signed-off-by: Erick Archer <erick.archer@outlook.com>
Thanks!
Reviewed-by: Kees Cook <keescook@chromium.org>
> --- > Changes in v3: > - Update the commit message to better explain the changes. > - Rebase against linux-next. > > Changes in v2: > - Add the "Reviewed-by:" tag. > - Rebase against linux-next. > > Previous versions: > v1 -> https://lore.kernel.org/linux-hardening/20240116125813.3754-1-erick.archer@gmx.com > v2 -> https://lore.kernel.org/linux-hardening/AS8PR02MB7237A07D73D6D15EBF72FD8D8B392@AS8PR02MB7237.eurprd02.prod.outlook.com > > Hi, > > This is a new try. In the v2 version Ingo explained that this change > is nonsense since kzalloc() is a perfectly usable interface and there > is no real overflow here. > > Anyway, if we have the 2-factor form of the allocator, I think it is > a good practice to use it. > > In this version I have updated the commit message to explain that > the code is obviusly safe in contrast with the last version where the > impression was given that there was a real overlow bug. > > I hope this patch can be applied this time. > > Regards, > Erick > --- > arch/x86/events/amd/uncore.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c > index 4ccb8fa483e6..61c0a2114183 100644 > --- a/arch/x86/events/amd/uncore.c > +++ b/arch/x86/events/amd/uncore.c > @@ -479,8 +479,8 @@ static int amd_uncore_ctx_init(struct amd_uncore *uncore, unsigned int cpu) > goto fail; > > curr->cpu = cpu; > - curr->events = kzalloc_node(sizeof(*curr->events) * > - pmu->num_counters, > + curr->events = kcalloc_node(pmu->num_counters, > + sizeof(*curr->events), > GFP_KERNEL, node);
As a general aside to the original code authors, looking at struct amd_uncore_pmu, I see stuff that should likely be u32 instead of "int". How is a negtaive num_counters ever sane?
struct amd_uncore_pmu { ... int num_counters; int rdpmc_base; u32 msr_base; int group; ... };
-- Kees Cook
| ![\](/images/icornerr.gif) |