[3/3] x86/ima: retry detecting secure boot mode
diff mbox series

Message ID 1542657371-7019-4-git-send-email-zohar@linux.ibm.com
State New
Headers show
Series
  • selftest/ima: fail kexec_load syscall
Related show

Commit Message

Mimi Zohar Nov. 19, 2018, 7:56 p.m. UTC
The secure boot mode may not be detected on boot for some reason (eg.
buggy firmware).  This patch attempts one more time to detect the
secure boot mode.

Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 arch/x86/kernel/Makefile   |  2 ++
 arch/x86/kernel/ima_arch.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/ima.h        |  2 +-
 3 files changed, 47 insertions(+), 3 deletions(-)

Comments

Matthew Garrett March 7, 2019, 10:28 p.m. UTC | #1
On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> The secure boot mode may not be detected on boot for some reason (eg.
> buggy firmware).  This patch attempts one more time to detect the
> secure boot mode.

Do we have cases where this has actually been seen? I'm not sure what
the circumstances are that would result in this behaviour.
Matthew Garrett March 7, 2019, 10:44 p.m. UTC | #2
On Thu, Mar 7, 2019 at 2:38 PM Justin Forbes <jforbes@redhat.com> wrote:
> On Thu, Mar 7, 2019 at 4:29 PM Matthew Garrett <mjg59@google.com> wrote:
>>
>> On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>> >
>> > The secure boot mode may not be detected on boot for some reason (eg.
>> > buggy firmware).  This patch attempts one more time to detect the
>> > secure boot mode.
>>
>> Do we have cases where this has actually been seen? I'm not sure what
>> the circumstances are that would result in this behaviour.
>
>
> We have never seen it in practice, though we only ever do anything with it with x86, so it is possible that some other platforms maybe?

I'm not sure that it buys us anything to check this in both the boot
stub and the running kernel. If a platform *is* giving us different
results, anything else relying on the information from the boot stub
is also going to be broken, so we should do this centrally rather than
in the IMA code.
Mimi Zohar March 7, 2019, 10:48 p.m. UTC | #3
On Thu, 2019-03-07 at 14:44 -0800, Matthew Garrett wrote:
> On Thu, Mar 7, 2019 at 2:38 PM Justin Forbes <jforbes@redhat.com> wrote:
> > On Thu, Mar 7, 2019 at 4:29 PM Matthew Garrett <mjg59@google.com> wrote:
> >>
> >> On Mon, Nov 19, 2018 at 11:57 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >> >
> >> > The secure boot mode may not be detected on boot for some reason (eg.
> >> > buggy firmware).  This patch attempts one more time to detect the
> >> > secure boot mode.
> >>
> >> Do we have cases where this has actually been seen? I'm not sure what
> >> the circumstances are that would result in this behaviour.
> >
> >
> > We have never seen it in practice, though we only ever do anything with it with x86, so it is possible that some other platforms maybe?
> 
> I'm not sure that it buys us anything to check this in both the boot
> stub and the running kernel. If a platform *is* giving us different
> results, anything else relying on the information from the boot stub
> is also going to be broken, so we should do this centrally rather than
> in the IMA code.

I added this last attempt because I'm seeing this on my laptop, with
some older, buggy firmware.

Mimi
Matthew Garrett March 7, 2019, 10:50 p.m. UTC | #4
On Thu, Mar 7, 2019 at 2:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> I added this last attempt because I'm seeing this on my laptop, with
> some older, buggy firmware.

Is the issue that it gives incorrect results on the first read, or is
the issue that it gives incorrect results before ExitBootServices() is
called? If the former then we should read twice in the boot stub, if
the latter then we should figure out a way to do this immediately
after ExitBootServices() instead.
Mimi Zohar March 8, 2019, 1:39 p.m. UTC | #5
On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> On Thu, Mar 7, 2019 at 2:48 PM Mimi Zohar <zohar@linux.ibm.com> wrote:
> > I added this last attempt because I'm seeing this on my laptop, with
> > some older, buggy firmware.
> 
> Is the issue that it gives incorrect results on the first read, or is
> the issue that it gives incorrect results before ExitBootServices() is
> called? If the former then we should read twice in the boot stub, if
> the latter then we should figure out a way to do this immediately
> after ExitBootServices() instead.

Detecting the secure boot mode isn't the problem.  On boot, I am
seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
"Secure boot could not be determined".

In efi_main() the secure_boot mode is initially unset, so
efi_get_secureboot() is called.  efi_get_secureboot() returns the
secure_boot mode correctly as enabled.  The problem seems to be in
saving the secure_boot mode for later use.

Mimi
Matthew Garrett March 8, 2019, 5:51 p.m. UTC | #6
On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > Is the issue that it gives incorrect results on the first read, or is
> > the issue that it gives incorrect results before ExitBootServices() is
> > called? If the former then we should read twice in the boot stub, if
> > the latter then we should figure out a way to do this immediately
> > after ExitBootServices() instead.
>
> Detecting the secure boot mode isn't the problem.  On boot, I am
> seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> "Secure boot could not be determined".
>
> In efi_main() the secure_boot mode is initially unset, so
> efi_get_secureboot() is called.  efi_get_secureboot() returns the
> secure_boot mode correctly as enabled.  The problem seems to be in
> saving the secure_boot mode for later use.

Hm. And this only happens on certain firmware versions? If something's
stepping on boot_params then we have bigger problems.
Mimi Zohar March 8, 2019, 6:43 p.m. UTC | #7
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > > Is the issue that it gives incorrect results on the first read, or is
> > > the issue that it gives incorrect results before ExitBootServices() is
> > > called? If the former then we should read twice in the boot stub, if
> > > the latter then we should figure out a way to do this immediately
> > > after ExitBootServices() instead.
> >
> > Detecting the secure boot mode isn't the problem.  On boot, I am
> > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> > "Secure boot could not be determined".
> >
> > In efi_main() the secure_boot mode is initially unset, so
> > efi_get_secureboot() is called.  efi_get_secureboot() returns the
> > secure_boot mode correctly as enabled.  The problem seems to be in
> > saving the secure_boot mode for later use.
> 
> Hm. And this only happens on certain firmware versions? If something's
> stepping on boot_params then we have bigger problems.

FYI, efi_printk() works before exit_boot(), but not afterwards.  The
system hangs.

Mimi
Matthew Garrett March 8, 2019, 8:22 p.m. UTC | #8
On Fri, Mar 8, 2019 at 10:43 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> FYI, efi_printk() works before exit_boot(), but not afterwards.  The
> system hangs.

efi_printk() uses boot services to print, so that's not unexpected :)
It would probably be sensible to return an error rather than crash,
though…
Mimi Zohar March 11, 2019, 4:54 p.m. UTC | #9
On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> On Fri, Mar 8, 2019 at 5:40 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Thu, 2019-03-07 at 14:50 -0800, Matthew Garrett wrote:
> > > Is the issue that it gives incorrect results on the first read, or is
> > > the issue that it gives incorrect results before ExitBootServices() is
> > > called? If the former then we should read twice in the boot stub, if
> > > the latter then we should figure out a way to do this immediately
> > > after ExitBootServices() instead.
> >
> > Detecting the secure boot mode isn't the problem.  On boot, I am
> > seeing "EFI stub: UEFI Secure Boot is enabled", but setup_arch() emits
> > "Secure boot could not be determined".
> >
> > In efi_main() the secure_boot mode is initially unset, so
> > efi_get_secureboot() is called.  efi_get_secureboot() returns the
> > secure_boot mode correctly as enabled.  The problem seems to be in
> > saving the secure_boot mode for later use.
> 
> Hm. And this only happens on certain firmware versions? If something's
> stepping on boot_params then we have bigger problems.

I was seeing this problem before and after updating the system
firmware on my laptop last summer.  If updating the firmware had
resolved the problem, I wouldn't have included this patch.

Mimi
Matthew Garrett March 11, 2019, 7:20 p.m. UTC | #10
On Mon, Mar 11, 2019 at 9:55 AM Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Fri, 2019-03-08 at 09:51 -0800, Matthew Garrett wrote:
> > Hm. And this only happens on certain firmware versions? If something's
> > stepping on boot_params then we have bigger problems.
>
> I was seeing this problem before and after updating the system
> firmware on my laptop last summer.  If updating the firmware had
> resolved the problem, I wouldn't have included this patch.

Ah, sorry, when you said that you saw this with older firmware I
thought that meant that newer firmware fixed it. What machine are you
testing on?

Patch
diff mbox series

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index f0910a1e1db7..eb51b0e1189c 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -151,4 +151,6 @@  ifeq ($(CONFIG_X86_64),y)
 	obj-y				+= vsmp_64.o
 endif
 
+ifdef CONFIG_EFI
 obj-$(CONFIG_IMA)			+= ima_arch.o
+endif
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
index 6c248616ee57..e47cd9390ab4 100644
--- a/arch/x86/kernel/ima_arch.c
+++ b/arch/x86/kernel/ima_arch.c
@@ -7,10 +7,52 @@ 
 
 extern struct boot_params boot_params;
 
+static enum efi_secureboot_mode get_sb_mode(void)
+{
+	efi_char16_t efi_SecureBoot_name[] = L"SecureBoot";
+	efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+	efi_status_t status;
+	unsigned long size;
+	u8 secboot;
+
+	size = sizeof(secboot);
+
+	/* Get variable contents into buffer */
+	status = efi.get_variable(efi_SecureBoot_name, &efi_variable_guid,
+				  NULL, &size, &secboot);
+	if (status == EFI_NOT_FOUND) {
+		pr_info("ima: secureboot mode disabled\n");
+		return efi_secureboot_mode_disabled;
+	}
+
+	if (status != EFI_SUCCESS) {
+		pr_info("ima: secureboot mode unknown\n");
+		return efi_secureboot_mode_unknown;
+	}
+
+	if (secboot == 0) {
+		pr_info("ima: secureboot mode disabled\n");
+		return efi_secureboot_mode_disabled;
+	}
+
+	pr_info("ima: secureboot mode enabled\n");
+	return efi_secureboot_mode_enabled;
+}
+
 bool arch_ima_get_secureboot(void)
 {
-	if (efi_enabled(EFI_BOOT) &&
-		(boot_params.secure_boot == efi_secureboot_mode_enabled))
+	static enum efi_secureboot_mode sb_mode;
+	static bool initialized;
+
+	if (!initialized && efi_enabled(EFI_BOOT)) {
+		sb_mode = boot_params.secure_boot;
+
+		if (sb_mode == efi_secureboot_mode_unset)
+			sb_mode = get_sb_mode();
+		initialized = true;
+	}
+
+	if (sb_mode == efi_secureboot_mode_enabled)
 		return true;
 	else
 		return false;
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 5ab9134d4fd7..b5e16b8c50b7 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,7 +30,7 @@  extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#ifdef CONFIG_X86
+#if defined(CONFIG_X86) && defined(CONFIG_EFI)
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else