| Message ID | 20231215122614.5481-3-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
| Series | arch/x86: Remove unnecessary dependencies on bootparam.h | expand |
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? > 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 >> >>
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
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) {
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