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:

quoted

Hi Takashi, Mimi,

On Tue, Dec 14, 2021 at 04:58:47PM +0100, Takashi Iwai wrote:

quoted

On Tue, 14 Dec 2021 16:31:21 +0100,
Mimi Zohar wrote:

quoted

Hi Takashi,

On Mon, 2021-12-13 at 17:11 +0100, Takashi Iwai wrote:

quoted

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 <redacted>

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