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 <[email protected]> > Signed-off-by: Erick Archer <[email protected]>
Thanks! Reviewed-by: Kees Cook <[email protected]> > --- > 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/[email protected] > v2 -> > https://lore.kernel.org/linux-hardening/as8pr02mb7237a07d73d6d15ebf72fd8d8b...@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
