| Message ID | AS8PR02MB7237A07D73D6D15EBF72FD8D8B392@AS8PR02MB7237.eurprd02.prod.outlook.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] perf/x86/amd/uncore: Use kcalloc*() instead of kzalloc*() | expand |
* Erick Archer <erick.archer@outlook.com> wrote: > As noted in the "Deprecated Interfaces, Language Features, Attributes, > and Conventions" documentation [1], size calculations (especially > multiplication) should not be performed in memory allocator (or similar) > function arguments due to the risk of them overflowing. This could lead > to values wrapping around and a smaller allocation being made than the > caller was expecting. Using those allocations could lead to linear > overflows of heap memory and other misbehaviors. > > So, use the purpose specific kcalloc*() function instead of the argument > size * count in the kzalloc*() function. > > [1] https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > Link: https://github.com/KSPP/linux/issues/162 > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > Signed-off-by: Erick Archer <erick.archer@outlook.com> > --- > 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/ > > Hi everyone, > > This patch seems to be lost. Gustavo reviewed it on January 16, 2024 > but the patch has not been applied since. > > Thanks, > 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); > if (!curr->events) { > kfree(curr); > @@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu) > uncore->num_pmus += group_num_pmus[gid]; > } > > - uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus, > + uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus), > GFP_KERNEL); > if (!uncore->pmus) { > uncore->num_pmus = 0; This change is nonsense, kzalloc() is a perfectly usable interface, and none of the arguments are user-controlled, so I don't see how there could be a real overflow bug here. Thanks, Ingo
Hi Ingo, On Sat, Mar 30, 2024 at 10:11:23PM +0100, Ingo Molnar wrote: > > * Erick Archer <erick.archer@outlook.com> wrote: > > > As noted in the "Deprecated Interfaces, Language Features, Attributes, > > and Conventions" documentation [1], size calculations (especially > > multiplication) should not be performed in memory allocator (or similar) > > function arguments due to the risk of them overflowing. This could lead > > to values wrapping around and a smaller allocation being made than the > > caller was expecting. Using those allocations could lead to linear > > overflows of heap memory and other misbehaviors. > > > > So, use the purpose specific kcalloc*() function instead of the argument > > size * count in the kzalloc*() function. > > > > [1] https://www.kernel.org/doc/html/next/process/deprecated.html#open-coded-arithmetic-in-allocator-arguments > > > > Link: https://github.com/KSPP/linux/issues/162 > > Reviewed-by: Gustavo A. R. Silva <gustavoars@kernel.org> > > Signed-off-by: Erick Archer <erick.archer@outlook.com> > > --- > > 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/ > > > > Hi everyone, > > > > This patch seems to be lost. Gustavo reviewed it on January 16, 2024 > > but the patch has not been applied since. > > > > Thanks, > > 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); > > if (!curr->events) { > > kfree(curr); > > @@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu) > > uncore->num_pmus += group_num_pmus[gid]; > > } > > > > - uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus, > > + uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus), > > GFP_KERNEL); > > if (!uncore->pmus) { > > uncore->num_pmus = 0; > > This change is nonsense, kzalloc() is a perfectly usable interface, and > none of the arguments are user-controlled, so I don't see how there > could be a real overflow bug here. Yes, you are right. This scenario is obviously safe, but this is an effort to get rid of all multiplications from allocations functions in the kernel. Surely, the commit message is scarier than it really is in reality. If you prefer I can send a v3 with the following commit message in order to better explain the change. [start of commit message] ----------------------------------------- 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] [end of commit message] ------------------------------------------- Best regards, Erick > Thanks, > > Ingo
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); if (!curr->events) { kfree(curr); @@ -928,7 +928,7 @@ int amd_uncore_umc_ctx_init(struct amd_uncore *uncore, unsigned int cpu) uncore->num_pmus += group_num_pmus[gid]; } - uncore->pmus = kzalloc(sizeof(*uncore->pmus) * uncore->num_pmus, + uncore->pmus = kcalloc(uncore->num_pmus, sizeof(*uncore->pmus), GFP_KERNEL); if (!uncore->pmus) { uncore->num_pmus = 0;