Re: [PATCH v1 3/9] virt: Implement Heki common code

[Wei Liu <wei.liu@xxxxxxxxxx>]

On Fri, May 05, 2023 at 05:20:40PM +0200, Mickaël Salaün wrote:
> From: Madhavan T. Venkataraman <madvenka@xxxxxxxxxxxxxxxxxxx>
> 
> Hypervisor Enforced Kernel Integrity (Heki) is a feature that will use
> the hypervisor to enhance guest virtual machine security.
> 
> Configuration
> =============
> 
> Define the config variables for the feature. This feature depends on
> support from the architecture as well as the hypervisor.
> 
> Enabling HEKI
> =============
> 
> Define a kernel command line parameter "heki" to turn the feature on or
> off. By default, Heki is on.

For such a newfangled feature can we have it off by default? Especially
when there are unsolved issues around dynamically loaded code.

> 
[...]
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3604074a878b..5cf5a7a97811 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -297,6 +297,7 @@ config X86
>       select FUNCTION_ALIGNMENT_4B
>       imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
>       select HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
> +     select ARCH_SUPPORTS_HEKI               if X86_64

Why is there a restriction on X86_64?

>  
>  config INSTRUCTION_DECODER
>       def_bool y
> diff --git a/arch/x86/include/asm/sections.h b/arch/x86/include/asm/sections.h
> index a6e8373a5170..42ef1e33b8a5 100644
> --- a/arch/x86/include/asm/sections.h
> +++ b/arch/x86/include/asm/sections.h
[...]
>  
> +#ifdef CONFIG_HEKI
> +
> +/*
> + * Gather all of the statically defined sections so heki_late_init() can
> + * protect these sections in the host page table.
> + *
> + * The sections are defined under "SECTIONS" in vmlinux.lds.S
> + * Keep this array in sync with SECTIONS.
> + */

This seems a bit fragile, because it requires constant attention from
people who care about this functionality. Can this table be
automatically generated?

Thanks,
Wei.

> +struct heki_va_range __initdata heki_va_ranges[] = {
> +     {
> +             .va_start = _stext,
> +             .va_end = _etext,
> +             .attributes = HEKI_ATTR_MEM_NOWRITE | HEKI_ATTR_MEM_EXEC,
> +     },
> +     {
> +             .va_start = __start_rodata,
> +             .va_end = __end_rodata,
> +             .attributes = HEKI_ATTR_MEM_NOWRITE,
> +     },
> +#ifdef CONFIG_UNWINDER_ORC
> +     {
> +             .va_start = __start_orc_unwind_ip,
> +             .va_end = __stop_orc_unwind_ip,
> +             .attributes = HEKI_ATTR_MEM_NOWRITE,
> +     },
> +     {
> +             .va_start = __start_orc_unwind,
> +             .va_end = __stop_orc_unwind,
> +             .attributes = HEKI_ATTR_MEM_NOWRITE,
> +     },
> +     {
> +             .va_start = orc_lookup,
> +             .va_end = orc_lookup_end,
> +             .attributes = HEKI_ATTR_MEM_NOWRITE,
> +     },
> +#endif /* CONFIG_UNWINDER_ORC */
> +};
> +