Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
defined only when CONFIG_IMA is set, and this makes the code calling
those functions without CONFIG_IMA failing. Although there is no such
in-tree users, but the out-of-tree users already hit it.
Move the declaration and the dummy definition of those functions
outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
Signed-off-by: Takashi Iwai <[email protected]>
---
include/linux/ima.h | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/include/linux/ima.h b/include/linux/ima.h
index b6ab66a546ae..426b1744215e 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -50,21 +50,6 @@ static inline void ima_appraise_parse_cmdline(void) {}
extern void ima_add_kexec_buffer(struct kimage *image);
#endif
-#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
-extern bool arch_ima_get_secureboot(void);
-extern const char * const *arch_get_ima_policy(void);
-#else
-static inline bool arch_ima_get_secureboot(void)
-{
- return false;
-}
-
-static inline const char * const *arch_get_ima_policy(void)
-{
- return NULL;
-}
-#endif
-
#else
static inline enum hash_algo ima_get_current_hash_algo(void)
{
@@ -155,6 +140,21 @@ static inline int ima_measure_critical_data(const char *event_label,
#endif /* CONFIG_IMA */
+#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
+extern bool arch_ima_get_secureboot(void);
+extern const char * const *arch_get_ima_policy(void);
+#else
+static inline bool arch_ima_get_secureboot(void)
+{
+ return false;
+}
+
+static inline const char * const *arch_get_ima_policy(void)
+{
+ return NULL;
+}
+#endif
+
#ifndef CONFIG_IMA_KEXEC
struct kimage;
--
2.31.1
Hi Takashi,
On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> defined only when CONFIG_IMA is set, and this makes the code calling
> those functions without CONFIG_IMA failing. Although there is no such
> in-tree users, but the out-of-tree users already hit it.
>
> Move the declaration and the dummy definition of those functions
> outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
>
> Signed-off-by: Takashi Iwai <[email protected]>
Before lockdown was upstreamed, we made sure that IMA and lockdown
could co-exist. This patch makes the stub functions available even
when IMA is not configured. Do the remaining downstream patches
require IMA to be disabled or can IMA co-exist?
thanks,
Mimi
On Tue, 14 Dec 2021 16:31:21 +0100,
Mimi Zohar wrote:
>
> Hi Takashi,
>
> On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > defined only when CONFIG_IMA is set, and this makes the code calling
> > those functions without CONFIG_IMA failing. Although there is no such
> > in-tree users, but the out-of-tree users already hit it.
> >
> > Move the declaration and the dummy definition of those functions
> > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> Before lockdown was upstreamed, we made sure that IMA and lockdown
> could co-exist. This patch makes the stub functions available even
> when IMA is not configured. Do the remaining downstream patches
> require IMA to be disabled or can IMA co-exist?
I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
used in a part of MODSIGN stuff in SUSE kernels, and it's calling
unconditionally this function for checking whether the system is with
the Secure Boot or not.
thanks,
Takashi
Hi Takashi, Mimi,
On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:
> On Tue, 14 Dec 2021 16:31:21 +0100,
> Mimi Zohar wrote:
> >
> > Hi Takashi,
> >
> > On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > > defined only when CONFIG_IMA is set, and this makes the code calling
> > > those functions without CONFIG_IMA failing. Although there is no such
> > > in-tree users, but the out-of-tree users already hit it.
> > >
> > > Move the declaration and the dummy definition of those functions
> > > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> > >
> > > Signed-off-by: Takashi Iwai <[email protected]>
> >
> > Before lockdown was upstreamed, we made sure that IMA and lockdown
> > could co-exist. This patch makes the stub functions available even
> > when IMA is not configured. Do the remaining downstream patches
> > require IMA to be disabled or can IMA co-exist?
>
> I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
> used in a part of MODSIGN stuff in SUSE kernels, and it's calling
> unconditionally this function for checking whether the system is with
> the Secure Boot or not.
>
Actually in downstream code, I used arch_ima_get_secureboot() in
load_uefi_certs() to prevent the mok be loaded when secure boot is
disabled. Because the security of MOK relies on secure boot.
The downstream code likes this:
/* the MOK and MOKx can not be trusted when secure boot is disabled */
- if (!efi_enabled(EFI_SECURE_BOOT))
+ if (!arch_ima_get_secureboot())
return 0;
The old EFI_SECURE_BOOT bit can only be available on x86_64, so I switch
the code to to arch_ima_get_secureboot() for cross-architectures and sync
with upstream api.
The load_uefi.c depends on CONFIG_INTEGRITY but not CONFIG_IMA. So
load_uefi.c still be built when CONFIG_INTEGRITY=y and CONFIG_IMA=n.
Then "implicit declaration of function 'arch_ima_get_secureboot'" is
happened.
Thanks a lot!
Joey Lee
[Cc'ing Eric Snowberg, Jarkko]
Hi Joey,
On Thu, 2021-12-16 at 00:03 +0800, joeyli wrote:
> Hi Takashi, Mimi,
>
> On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:
> > On Tue, 14 Dec 2021 16:31:21 +0100,
> > Mimi Zohar wrote:
> > >
> > > Hi Takashi,
> > >
> > > On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > > > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > > > defined only when CONFIG_IMA is set, and this makes the code calling
> > > > those functions without CONFIG_IMA failing. Although there is no such
> > > > in-tree users, but the out-of-tree users already hit it.
> > > >
> > > > Move the declaration and the dummy definition of those functions
> > > > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> > > >
> > > > Signed-off-by: Takashi Iwai <[email protected]>
> > >
> > > Before lockdown was upstreamed, we made sure that IMA and lockdown
> > > could co-exist. This patch makes the stub functions available even
> > > when IMA is not configured. Do the remaining downstream patches
> > > require IMA to be disabled or can IMA co-exist?
> >
> > I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
> > used in a part of MODSIGN stuff in SUSE kernels, and it's calling
> > unconditionally this function for checking whether the system is with
> > the Secure Boot or not.
> >
>
> Actually in downstream code, I used arch_ima_get_secureboot() in
> load_uefi_certs() to prevent the mok be loaded when secure boot is
> disabled. Because the security of MOK relies on secure boot.
>
> The downstream code likes this:
>
> /* the MOK and MOKx can not be trusted when secure boot is disabled */
> - if (!efi_enabled(EFI_SECURE_BOOT))
> + if (!arch_ima_get_secureboot())
> return 0;
>
> The old EFI_SECURE_BOOT bit can only be available on x86_64, so I switch
> the code to to arch_ima_get_secureboot() for cross-architectures and sync
> with upstream api.
>
> The load_uefi.c depends on CONFIG_INTEGRITY but not CONFIG_IMA. So
> load_uefi.c still be built when CONFIG_INTEGRITY=y and CONFIG_IMA=n.
> Then "implicit declaration of function 'arch_ima_get_secureboot'" is
> happened.
The existing upstream code doesn't require secureboot to load the
MOK/MOKx keys. Why is your change being made downstream?
Are you aware of Eric Snowberg's "Enroll kernel keys thru MOK" patch
set? When INTEGRITY_MACHINE_KEYRING is enabled and new UEFI variables
are enabled, instead of loading the MOK keys onto the .platform
keyring, they're loaded onto a new keyring name ".machine", which is
linked to the secondary keyring.
Eric's patchset doesn't add a new check either to make sure secure boot
is enabled before loading the MOK/MOKx keys.
thanks,
Mimi
On Wed, Dec 15, 2021 at 01:16:48PM -0500, Mimi Zohar wrote:
> [Cc'ing Eric Snowberg, Jarkko]
>
> Hi Joey,
>
> On Thu, 2021-12-16 at 00:03 +0800, joeyli wrote:
> > Hi Takashi, Mimi,
> >
> > On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:
> > > On Tue, 14 Dec 2021 16:31:21 +0100,
> > > Mimi Zohar wrote:
> > > >
> > > > Hi Takashi,
> > > >
> > > > On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > > > > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > > > > defined only when CONFIG_IMA is set, and this makes the code calling
> > > > > those functions without CONFIG_IMA failing. Although there is no such
> > > > > in-tree users, but the out-of-tree users already hit it.
> > > > >
> > > > > Move the declaration and the dummy definition of those functions
> > > > > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> > > > >
> > > > > Signed-off-by: Takashi Iwai <[email protected]>
> > > >
> > > > Before lockdown was upstreamed, we made sure that IMA and lockdown
> > > > could co-exist. This patch makes the stub functions available even
> > > > when IMA is not configured. Do the remaining downstream patches
> > > > require IMA to be disabled or can IMA co-exist?
> > >
> > > I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
> > > used in a part of MODSIGN stuff in SUSE kernels, and it's calling
> > > unconditionally this function for checking whether the system is with
> > > the Secure Boot or not.
> > >
> >
> > Actually in downstream code, I used arch_ima_get_secureboot() in
> > load_uefi_certs() to prevent the mok be loaded when secure boot is
> > disabled. Because the security of MOK relies on secure boot.
> >
> > The downstream code likes this:
> >
> > /* the MOK and MOKx can not be trusted when secure boot is disabled */
> > - if (!efi_enabled(EFI_SECURE_BOOT))
> > + if (!arch_ima_get_secureboot())
> > return 0;
> >
> > The old EFI_SECURE_BOOT bit can only be available on x86_64, so I switch
> > the code to to arch_ima_get_secureboot() for cross-architectures and sync
> > with upstream api.
> >
> > The load_uefi.c depends on CONFIG_INTEGRITY but not CONFIG_IMA. So
> > load_uefi.c still be built when CONFIG_INTEGRITY=y and CONFIG_IMA=n.
> > Then "implicit declaration of function 'arch_ima_get_secureboot'" is
> > happened.
>
> The existing upstream code doesn't require secureboot to load the
> MOK/MOKx keys. Why is your change being made downstream?
>
Because the security of MOK relies on secure boot. When secure boot is
disabled, EFI firmware will not verify binary code. So arbitrary efi
binary code can modify MOK when rebooting.
When user disabled secure boot, a hacker can just replace shim.efi then
reboot for enrolling new MOK without any interactive. Or hacker can just
replace shim.efi and wait user to reboot their machine. A hacker's MOK can
also be enrolled by hacked shim. User can't perceive.
> Are you aware of Eric Snowberg's "Enroll kernel keys thru MOK" patch
> set? When INTEGRITY_MACHINE_KEYRING is enabled and new UEFI variables
> are enabled, instead of loading the MOK keys onto the .platform
> keyring, they're loaded onto a new keyring name ".machine", which is
> linked to the secondary keyring.
>
> Eric's patchset doesn't add a new check either to make sure secure boot
> is enabled before loading the MOK/MOKx keys.
>
Kernel doesn't know how was a MOK enrolled. Kernel can only detect the
state of secure boot. If kernel doesn't want to check the state of secure
boot before loading MOK, then user should understands that they can not disable
secure boot when using MOK.
Thanks a lot!
Joey Lee
On Thu, 2021-12-16 at 12:32 +0800, joeyli wrote:
> On Wed, Dec 15, 2021 at 01:16:48PM -0500, Mimi Zohar wrote:
> > [Cc'ing Eric Snowberg, Jarkko]
> >
> > Hi Joey,
> >
> > On Thu, 2021-12-16 at 00:03 +0800, joeyli wrote:
> > > Hi Takashi, Mimi,
> > >
> > > On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:
> > > > On Tue, 14 Dec 2021 16:31:21 +0100,
> > > > Mimi Zohar wrote:
> > > > >
> > > > > Hi Takashi,
> > > > >
> > > > > On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > > > > > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > > > > > defined only when CONFIG_IMA is set, and this makes the code calling
> > > > > > those functions without CONFIG_IMA failing. Although there is no such
> > > > > > in-tree users, but the out-of-tree users already hit it.
> > > > > >
> > > > > > Move the declaration and the dummy definition of those functions
> > > > > > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> > > > > >
> > > > > > Signed-off-by: Takashi Iwai <[email protected]>
> > > > >
> > > > > Before lockdown was upstreamed, we made sure that IMA and lockdown
> > > > > could co-exist. This patch makes the stub functions available even
> > > > > when IMA is not configured. Do the remaining downstream patches
> > > > > require IMA to be disabled or can IMA co-exist?
> > > >
> > > > I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
> > > > used in a part of MODSIGN stuff in SUSE kernels, and it's calling
> > > > unconditionally this function for checking whether the system is with
> > > > the Secure Boot or not.
> > > >
> > >
> > > Actually in downstream code, I used arch_ima_get_secureboot() in
> > > load_uefi_certs() to prevent the mok be loaded when secure boot is
> > > disabled. Because the security of MOK relies on secure boot.
> > >
> > > The downstream code likes this:
> > >
> > > /* the MOK and MOKx can not be trusted when secure boot is disabled */
> > > - if (!efi_enabled(EFI_SECURE_BOOT))
> > > + if (!arch_ima_get_secureboot())
> > > return 0;
> > >
> > > The old EFI_SECURE_BOOT bit can only be available on x86_64, so I switch
> > > the code to to arch_ima_get_secureboot() for cross-architectures and sync
> > > with upstream api.
> > >
> > > The load_uefi.c depends on CONFIG_INTEGRITY but not CONFIG_IMA. So
> > > load_uefi.c still be built when CONFIG_INTEGRITY=y and CONFIG_IMA=n.
> > > Then "implicit declaration of function 'arch_ima_get_secureboot'" is
> > > happened.
> >
> > The existing upstream code doesn't require secureboot to load the
> > MOK/MOKx keys. Why is your change being made downstream?
> >
> Because the security of MOK relies on secure boot. When secure boot is
> disabled, EFI firmware will not verify binary code. So arbitrary efi
> binary code can modify MOK when rebooting.
>
> When user disabled secure boot, a hacker can just replace shim.efi then
> reboot for enrolling new MOK without any interactive. Or hacker can just
> replace shim.efi and wait user to reboot their machine. A hacker's MOK can
> also be enrolled by hacked shim. User can't perceive.
>
> > Are you aware of Eric Snowberg's "Enroll kernel keys thru MOK" patch
> > set? When INTEGRITY_MACHINE_KEYRING is enabled and new UEFI variables
> > are enabled, instead of loading the MOK keys onto the .platform
> > keyring, they're loaded onto a new keyring name ".machine", which is
> > linked to the secondary keyring.
> >
> > Eric's patchset doesn't add a new check either to make sure secure boot
> > is enabled before loading the MOK/MOKx keys.
> >
>
> Kernel doesn't know how was a MOK enrolled. Kernel can only detect the
> state of secure boot. If kernel doesn't want to check the state of secure
> boot before loading MOK, then user should understands that they can not disable
> secure boot when using MOK.
Thanks, Joey, for the detailed explained. I was agreeing with you that
MOK/MOKx keys should only be loaded when secure boot is enabled. A
better way for me to have phrased the questions would have been, why
are you making this change downstream and not upstream?
thanks,
Mimi
On Thu, Dec 16, 2021 at 08:22:56AM -0500, Mimi Zohar wrote:
> On Thu, 2021-12-16 at 12:32 +0800, joeyli wrote:
> > On Wed, Dec 15, 2021 at 01:16:48PM -0500, Mimi Zohar wrote:
> > > [Cc'ing Eric Snowberg, Jarkko]
> > >
> > > Hi Joey,
> > >
> > > On Thu, 2021-12-16 at 00:03 +0800, joeyli wrote:
> > > > Hi Takashi, Mimi,
> > > >
> > > > On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:
> > > > > On Tue, 14 Dec 2021 16:31:21 +0100,
> > > > > Mimi Zohar wrote:
> > > > > >
> > > > > > Hi Takashi,
> > > > > >
> > > > > > On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > > > > > > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > > > > > > defined only when CONFIG_IMA is set, and this makes the code calling
> > > > > > > those functions without CONFIG_IMA failing. Although there is no such
> > > > > > > in-tree users, but the out-of-tree users already hit it.
> > > > > > >
> > > > > > > Move the declaration and the dummy definition of those functions
> > > > > > > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> > > > > > >
> > > > > > > Signed-off-by: Takashi Iwai <[email protected]>
> > > > > >
> > > > > > Before lockdown was upstreamed, we made sure that IMA and lockdown
> > > > > > could co-exist. This patch makes the stub functions available even
> > > > > > when IMA is not configured. Do the remaining downstream patches
> > > > > > require IMA to be disabled or can IMA co-exist?
> > > > >
> > > > > I guess Joey (Cc'ed) can explain this better. AFAIK, currently it's
> > > > > used in a part of MODSIGN stuff in SUSE kernels, and it's calling
> > > > > unconditionally this function for checking whether the system is with
> > > > > the Secure Boot or not.
> > > > >
> > > >
> > > > Actually in downstream code, I used arch_ima_get_secureboot() in
> > > > load_uefi_certs() to prevent the mok be loaded when secure boot is
> > > > disabled. Because the security of MOK relies on secure boot.
> > > >
> > > > The downstream code likes this:
> > > >
> > > > /* the MOK and MOKx can not be trusted when secure boot is disabled */
> > > > - if (!efi_enabled(EFI_SECURE_BOOT))
> > > > + if (!arch_ima_get_secureboot())
> > > > return 0;
> > > >
> > > > The old EFI_SECURE_BOOT bit can only be available on x86_64, so I switch
> > > > the code to to arch_ima_get_secureboot() for cross-architectures and sync
> > > > with upstream api.
> > > >
> > > > The load_uefi.c depends on CONFIG_INTEGRITY but not CONFIG_IMA. So
> > > > load_uefi.c still be built when CONFIG_INTEGRITY=y and CONFIG_IMA=n.
> > > > Then "implicit declaration of function 'arch_ima_get_secureboot'" is
> > > > happened.
> > >
> > > The existing upstream code doesn't require secureboot to load the
> > > MOK/MOKx keys. Why is your change being made downstream?
> > >
> > Because the security of MOK relies on secure boot. When secure boot is
> > disabled, EFI firmware will not verify binary code. So arbitrary efi
> > binary code can modify MOK when rebooting.
> >
> > When user disabled secure boot, a hacker can just replace shim.efi then
> > reboot for enrolling new MOK without any interactive. Or hacker can just
> > replace shim.efi and wait user to reboot their machine. A hacker's MOK can
> > also be enrolled by hacked shim. User can't perceive.
> >
> > > Are you aware of Eric Snowberg's "Enroll kernel keys thru MOK" patch
> > > set? When INTEGRITY_MACHINE_KEYRING is enabled and new UEFI variables
> > > are enabled, instead of loading the MOK keys onto the .platform
> > > keyring, they're loaded onto a new keyring name ".machine", which is
> > > linked to the secondary keyring.
> > >
> > > Eric's patchset doesn't add a new check either to make sure secure boot
> > > is enabled before loading the MOK/MOKx keys.
> > >
> >
> > Kernel doesn't know how was a MOK enrolled. Kernel can only detect the
> > state of secure boot. If kernel doesn't want to check the state of secure
> > boot before loading MOK, then user should understands that they can not disable
> > secure boot when using MOK.
>
> Thanks, Joey, for the detailed explained. I was agreeing with you that
> MOK/MOKx keys should only be loaded when secure boot is enabled. A
> better way for me to have phrased the questions would have been, why
> are you making this change downstream and not upstream?
>
I just sent patch. The patch may changes behavior of kernel functions who
used MOK/MOKx. We can discuss in that patch.
Thanks
Joey Lee
Hi Takashi,
On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> defined only when CONFIG_IMA is set, and this makes the code calling
> those functions without CONFIG_IMA failing. Although there is no such
> in-tree users, but the out-of-tree users already hit it.
>
> Move the declaration and the dummy definition of those functions
> outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
>
> Signed-off-by: Takashi Iwai <[email protected]>
Joey's patch has a dependency on your patch, as seen by the kernel test
robot report. I'll drop the one line referencing in-tree/out-of-tree
sentence in the patch description, before picking it up as well.
thanks,
Mimi
On Wed, 22 Dec 2021 18:10:05 +0100,
Mimi Zohar wrote:
>
> Hi Takashi,
>
> On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:
> > Currently arch_ima_get_secureboot() and arch_get_ima_policy() are
> > defined only when CONFIG_IMA is set, and this makes the code calling
> > those functions without CONFIG_IMA failing. Although there is no such
> > in-tree users, but the out-of-tree users already hit it.
> >
> > Move the declaration and the dummy definition of those functions
> > outside ifdef-CONFIG_IMA block for fixing the undefined symbols.
> >
> > Signed-off-by: Takashi Iwai <[email protected]>
>
> Joey's patch has a dependency on your patch, as seen by the kernel test
> robot report. I'll drop the one line referencing in-tree/out-of-tree
> sentence in the patch description, before picking it up as well.
Sure, feel free to cook it :)
thanks,
Takashi