On Mon, Aug 17, 2020 at 05:34:23PM +0530, Anshuman Khandual wrote:
> HWCAP name arrays (hwcap_str, compat_hwcap_str, compat_hwcap2_str) that are
> scanned for /proc/cpuinfo are detached from their bit definitions making it
> vulnerable and difficult to correlate. It is also bit problematic because
> during /proc/cpuinfo dump these arrays get traversed sequentially assuming
> they reflect and match actual HWCAP bit sequence, to test various features
> for a given CPU. This redefines name arrays per their HWCAP bit definitions
> . It also warns after detecting any feature which is not expected on arm64.
>
> Cc: Catalin Marinas <catalin.mari...@arm.com>
> Cc: Will Deacon <w...@kernel.org>
> Cc: Mark Brown <broo...@kernel.org>
> Cc: Dave Martin <dave.mar...@arm.com>
> Cc: Ard Biesheuvel <a...@kernel.org>
> Cc: Mark Rutland <mark.rutl...@arm.com>
> Cc: Suzuki K Poulose <suzuki.poul...@arm.com>
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khand...@arm.com>
> ---
> This applies on 5.9-rc1
>
> Mark, since the patch has changed I have dropped your Acked-by: tag. Are you
> happy to give a new one ?
>
> Changes in V3:
>
> - Moved name arrays to (arch/arm64/kernel/cpuinfo.c) to prevent a build
> warning
> - Replaced string values with NULL for all compat features not possible on
> arm64
> - Changed compat_hwcap_str[] iteration on size as some NULL values are
> expected
> - Warn once after detecting any feature on arm64 that is not expected
>
> Changes in V2: (https://patchwork.kernel.org/patch/11533755/)
>
> - Defined COMPAT_KERNEL_HWCAP[2] and updated the name arrays per Mark
> - Updated the commit message as required
>
> Changes in V1: (https://patchwork.kernel.org/patch/11532945/)
>
> arch/arm64/include/asm/hwcap.h | 9 +++
> arch/arm64/kernel/cpuinfo.c | 172
> ++++++++++++++++++++++-------------------
> 2 files changed, 100 insertions(+), 81 deletions(-)
[...]
> + [KERNEL_HWCAP_FP] = "fp",
> + [KERNEL_HWCAP_ASIMD] = "asimd",
> + [KERNEL_HWCAP_EVTSTRM] = "evtstrm",
> + [KERNEL_HWCAP_AES] = "aes",
It would be nice if the cap and the string were generated by the same
macro, along the lines of:
#define KERNEL_HWCAP(c) [KERNEL_HWCAP_##c] = #c,
Does making the constants mixed case break anything, or is it just really
churny to do?
> @@ -166,9 +167,18 @@ static int c_show(struct seq_file *m, void *v)
> seq_puts(m, "Features\t:");
> if (compat) {
> #ifdef CONFIG_COMPAT
> - for (j = 0; compat_hwcap_str[j]; j++)
> - if (compat_elf_hwcap & (1 << j))
> + for (j = 0; j < ARRAY_SIZE(compat_hwcap_str); j++) {
> + if (compat_elf_hwcap & (1 << j)) {
> + /*
> + * Warn once if any feature should not
> + * have been present on arm64 platform.
> + */
> + if (WARN_ON_ONCE(!compat_hwcap_str[j]))
> + continue;
> +
> seq_printf(m, " %s",
> compat_hwcap_str[j]);
> + }
> + }
>
> for (j = 0; compat_hwcap2_str[j]; j++)
Hmm, I find this pretty confusing now as compat_hwcap_str is not NULL
terminated and must be traversed with a loop bounded by ARRAY_SIZE(...),
whereas compat_hwcap2_str *is* NULL terminated and is traversed until you
hit the sentinel.
I think hwcap_str, compat_hwcap_str and compat_hwcap2_str should be
identical in this regard.
Will
