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.
v2:
* remove extra declaration of boot_params (Ard)
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.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.0Hi Thomas,
On Fri, 15 Dec 2023 at 13:26, Thomas Zimmermann <tzimmermann@suse.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@suse.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
>
>
Hii Ard
Am 19.12.23 um 12:38 schrieb Ard Biesheuvel:
> Hi Thomas,
>
> On Fri, 15 Dec 2023 at 13:26, Thomas Zimmermann <tzimmermann@suse.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?
I cannot figure out how to do this without *something* in asm-generic or
adding if-CONFIG_X86 guards in ima-efi.c.
But I noticed that linux/efi.h already contains 2 or 3 ifdef branches
for x86. Would it be an option to move this code into asm/efi.h
(including a header file in asm-generic for the non-x86 variants) and
add the arch_ima_efi_boot_mode() helper there as well? At least that
wouldn't be a header for only a single define.
Best regards
Thomas
>
>
>
>
>
>> 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@suse.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
>>
>>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
On Tue, 2 Jan 2024 at 15:07, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hii Ard
>
> Am 19.12.23 um 12:38 schrieb Ard Biesheuvel:
> > Hi Thomas,
> >
> > On Fri, 15 Dec 2023 at 13:26, Thomas Zimmermann <tzimmermann@suse.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?
>
> I cannot figure out how to do this without *something* in asm-generic or
> adding if-CONFIG_X86 guards in ima-efi.c.
>
> But I noticed that linux/efi.h already contains 2 or 3 ifdef branches
> for x86. Would it be an option to move this code into asm/efi.h
> (including a header file in asm-generic for the non-x86 variants) and
> add the arch_ima_efi_boot_mode() helper there as well? At least that
> wouldn't be a header for only a single define.
>
Could we just move the x86 implementation out of line?
So something like this in arch/x86/include/asm/efi.h
enum efi_secureboot_mode x86_ima_efi_boot_mode(void);
#define arch_ima_efi_boot_mode x86_ima_efi_boot_mode()
and an implementation in one of the related .c files:
enum efi_secureboot_mode x86_ima_efi_boot_mode(void)
{
return boot_params.secure_boot;
}
?
Hi
Am 03.01.24 um 14:11 schrieb Ard Biesheuvel:
> On Tue, 2 Jan 2024 at 15:07, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> Hii Ard
>>
>> Am 19.12.23 um 12:38 schrieb Ard Biesheuvel:
>>> Hi Thomas,
>>>
>>> On Fri, 15 Dec 2023 at 13:26, Thomas Zimmermann <tzimmermann@suse.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?
>>
>> I cannot figure out how to do this without *something* in asm-generic or
>> adding if-CONFIG_X86 guards in ima-efi.c.
>>
>> But I noticed that linux/efi.h already contains 2 or 3 ifdef branches
>> for x86. Would it be an option to move this code into asm/efi.h
>> (including a header file in asm-generic for the non-x86 variants) and
>> add the arch_ima_efi_boot_mode() helper there as well? At least that
>> wouldn't be a header for only a single define.
>>
>
> Could we just move the x86 implementation out of line?
>
> So something like this in arch/x86/include/asm/efi.h
>
> enum efi_secureboot_mode x86_ima_efi_boot_mode(void);
> #define arch_ima_efi_boot_mode x86_ima_efi_boot_mode()
>
> and an implementation in one of the related .c files:
>
> enum efi_secureboot_mode x86_ima_efi_boot_mode(void)
> {
> return boot_params.secure_boot;
> }
>
> ?
Well, that's just enough to avoid boot_params within the header file.
But it should work.
Best regards
Thomas
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
© 2016 - 2025 Red Hat, Inc.