Re: [PATCH v4 0/3] Hardening perf subsystem

[Kees Cook]

On Sat, Jun 01, 2024 at 06:56:15PM +0200, Erick Archer wrote:
> Hi everyone,
> 
> This is an effort to get rid of all multiplications from allocation
> functions in order to prevent integer overflows [1][2].

I didn't actually see these 3 patches in this thread nor via lore.

> In the first patch, the "struct amd_uncore_ctx" can be refactored to
> use a flex array for the "events" member. This way, the allocation/
> freeing of the memory can be simplified. Then, the struct_size()
> helper can be used to do the arithmetic calculation for the memory
> to be allocated.

I like this patch because it reduces the allocation from 2 to 1. This
isn't what Peter might see as "churn": this is an improvement in resource
utilization.

I think it's here:
https://lore.kernel.org/linux-hardening/AS8PR02MB7237E4848B44A5226BD3551C8BE02@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

> In the second patch, as the "struct intel_uncore_box" ends in a
> flexible array, 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_node() function.

This is
https://lore.kernel.org/linux-hardening/AS8PR02MB7237F4D39BF6AA0FF40E91638B392@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

I prefer this style, as it makes things unambiguous ("this will never
wrap around") without having to check the associated types and doesn't make
the resulting binary code different in the "can never overflow" case.

In this particular case:

int size = sizeof(*box) + numshared * sizeof(struct intel_uncore_extra_reg);

"int numshared" comes from struct intel_uncore_type::num_shared_regs,
which is:

        unsigned num_shared_regs:8;

And the struct sizes are:

$ pahole -C intel_uncore_box gcc-boot/vmlinux | grep size:
        /* size: 488, cachelines: 8, members: 19 */
$ pahole -C intel_uncore_extra_reg gcc-boot/vmlinux | grep size:
        /* size: 96, cachelines: 2, members: 5 */

So we have:

s32 size = 488 + u8 * 96

Max size here is 24968 so it can never overflow an s32, so I can see
why Peter views this as "churn".

I still think the patch is a coding style improvement, but okay.

> In the third patch, as the "struct perf_buffer" also ends in a
> flexible array, 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_node() functions. At the same
> time, prepare for the coming implementation by GCC and Clang of the
> __counted_by attribute.

This is
https://lore.kernel.org/linux-hardening/AS8PR02MB72379B4807F3951A1B926BA58BE02@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

This provides __counted_by coverage, and I think this is important to
gain in ever place we can. Given that this is part of a ring buffer
implementation that is arbitrarily sized, this is exactly the kind of
place I'd like to see __counted_by used. This is a runtime robustness
improvement, so I don't see this a "churn" at all.


Peter, for patches 1 and 3, if you'd prefer not to carry them, I could
put them in the hardening tree to keep them out of your way. It seems
clear you don't want patch 2 at all.

-Kees

-- 
Kees Cook