linux-kernel - Re: [PATCH v2 2/3] arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMj1kXHrn-PxpMGnR4VoHv7kHvQYyf8SS9i1irm9Gi_uBseciw@mail.gmail.com>
Date: Tue, 19 Dec 2023 12:38:44 +0100
From: Ard Biesheuvel <ardb@...nel.org>
To: Thomas Zimmermann <tzimmermann@...e.de>
Cc: tglx@...utronix.de, mingo@...hat.com, bp@...en8.de, 
	dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com, 
	bhelgaas@...gle.com, arnd@...db.de, zohar@...ux.ibm.com, 
	dmitry.kasatkin@...il.com, paul@...l-moore.com, jmorris@...ei.org, 
	serge@...lyn.com, javierm@...hat.com, linux-arch@...r.kernel.org, 
	linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-pci@...r.kernel.org, linux-integrity@...r.kernel.org, 
	linux-security-module@...r.kernel.org
Subject: Re: [PATCH v2 2/3] arch/x86: Add <asm/ima-efi.h> for arch_ima_efi_boot_mode

Hi Thomas,

On Fri, 15 Dec 2023 at 13:26, Thomas Zimmermann <tzimmermann@...e.de> wrote:
>
> The header file <asm/efi.h> contains the macro arch_ima_efi_boot_mode,
> which expands to use struct boot_params from <asm/bootparams.h>. Many
> drivers include <linux/efi.h>, but do not use boot parameters. Changes
> to bootparam.h or its included headers can easily trigger large,
> unnessary rebuilds of the kernel.
>
> Moving x86's arch_ima_efi_boot_mode to <asm/ima-efi.h> and including
> <asm/setup.h> separates that dependency from the rest of the EFI
> interfaces. The only user is in ima_efi.c. As the file already declares
> a default value for arch_ima_efi_boot_mode, move this define into
> asm-generic for all other architectures.
>
> With arch_ima_efi_boot_mode removed from efi.h, <asm/bootparam.h> can
> later be removed from further x86 header files.
>

Apologies if I missed this in v1 but is the new asm-generic header
really necessary? Could we instead turn arch_ima_efi_boot_mode into a
function that is a static inline { return unset; } by default, but can
be emitted out of line in one of the x86/platform/efi.c source files,
where referring to boot_params is fine?





> v2:
>         * remove extra declaration of boot_params (Ard)
>

Please don't put the revision log here, but below the --- so that 'git
am' will ignore it.


> Signed-off-by: Thomas Zimmermann <tzimmermann@...e.de>
> ---
>  arch/x86/include/asm/efi.h       |  3 ---
>  arch/x86/include/asm/ima-efi.h   | 11 +++++++++++
>  include/asm-generic/Kbuild       |  1 +
>  include/asm-generic/ima-efi.h    | 16 ++++++++++++++++
>  security/integrity/ima/ima_efi.c |  5 +----
>  5 files changed, 29 insertions(+), 7 deletions(-)
>  create mode 100644 arch/x86/include/asm/ima-efi.h
>  create mode 100644 include/asm-generic/ima-efi.h
>
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index c4555b269a1b..99f31176c892 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -418,9 +418,6 @@ extern int __init efi_memmap_split_count(efi_memory_desc_t *md,
>  extern void __init efi_memmap_insert(struct efi_memory_map *old_memmap,
>                                      void *buf, struct efi_mem_range *mem);
>
> -#define arch_ima_efi_boot_mode \
> -       ({ extern struct boot_params boot_params; boot_params.secure_boot; })
> -
>  #ifdef CONFIG_EFI_RUNTIME_MAP
>  int efi_get_runtime_map_size(void);
>  int efi_get_runtime_map_desc_size(void);
> diff --git a/arch/x86/include/asm/ima-efi.h b/arch/x86/include/asm/ima-efi.h
> new file mode 100644
> index 000000000000..b4d904e66b39
> --- /dev/null
> +++ b/arch/x86/include/asm/ima-efi.h
> @@ -0,0 +1,11 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_IMA_EFI_H
> +#define _ASM_X86_IMA_EFI_H
> +
> +#include <asm/setup.h>
> +
> +#define arch_ima_efi_boot_mode boot_params.secure_boot
> +
> +#include <asm-generic/ima-efi.h>
> +
> +#endif /* _ASM_X86_IMA_EFI_H */
> diff --git a/include/asm-generic/Kbuild b/include/asm-generic/Kbuild
> index def242528b1d..4fd16e71e8cd 100644
> --- a/include/asm-generic/Kbuild
> +++ b/include/asm-generic/Kbuild
> @@ -26,6 +26,7 @@ mandatory-y += ftrace.h
>  mandatory-y += futex.h
>  mandatory-y += hardirq.h
>  mandatory-y += hw_irq.h
> +mandatory-y += ima-efi.h
>  mandatory-y += io.h
>  mandatory-y += irq.h
>  mandatory-y += irq_regs.h
> diff --git a/include/asm-generic/ima-efi.h b/include/asm-generic/ima-efi.h
> new file mode 100644
> index 000000000000..f87f5edef440
> --- /dev/null
> +++ b/include/asm-generic/ima-efi.h
> @@ -0,0 +1,16 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef __ASM_GENERIC_IMA_EFI_H_
> +#define __ASM_GENERIC_IMA_EFI_H_
> +
> +#include <linux/efi.h>
> +
> +/*
> + * Only include this header file from your architecture's <asm/ima-efi.h>.
> + */
> +
> +#ifndef arch_ima_efi_boot_mode
> +#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
> +#endif
> +
> +#endif /* __ASM_GENERIC_FB_H_ */
> diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
> index 138029bfcce1..56bbee271cec 100644
> --- a/security/integrity/ima/ima_efi.c
> +++ b/security/integrity/ima/ima_efi.c
> @@ -6,10 +6,7 @@
>  #include <linux/module.h>
>  #include <linux/ima.h>
>  #include <asm/efi.h>
> -
> -#ifndef arch_ima_efi_boot_mode
> -#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
> -#endif
> +#include <asm/ima-efi.h>
>
>  static enum efi_secureboot_mode get_sb_mode(void)
>  {
> --
> 2.43.0
>
>