Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653492 > unrolled thread

Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-05-30 21:00 +0200
    Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode David Howells <dhowells@redhat.com> - 2017-05-31 11:30 +0200
      Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-05-31 13:40 +0200
        Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode David Howells <dhowells@redhat.com> - 2017-05-31 15:40 +0200
          Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2017-05-31 16:10 +0200
        Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode David Howells <dhowells@redhat.com> - 2017-06-06 11:40 +0200

#1653492 — Re: [PATCH 0/5] security, efi: Set lockdown if in secure boot mode

On 24 May 2017 at 14:45, David Howells <dhowells@redhat.com> wrote:
>
> Here's a set of patches to institute a "locked-down mode" in the kernel and
> to set that mode if the kernel is booted in secure-boot mode.  This can be
> enabled with CONFIG_LOCK_DOWN_KERNEL.  If a kernel is locked down, the
> lockdown can be lifted by typing SysRq+x on a keyboard attached to the
> machine if CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT is enabled.  The exact key can
> be configured as 'x' is already taken on some arches.
>
> Inside the kernel, kernel_is_locked_down() is used to check if the kernel
> is in lockdown mode.  In lock-down mode, at least the following
> restrictions will need to be emplaced:
>
>  (1) No unsigned modules, kexec images or firmware.
>
>  (2) No direct read/write access of the kernel image.  (Shouldn't be able
>      to modify it and shouldn't be able to read out crypto data).
>
>  (3) No direct access to devices.  (DMA could be used to access/modify the
>      kernel image).
>
>  (4) No manual setting of device register addresses to cause a driver for
>      one device to mess around with another device, thereby permitting DMA.
>
>  (5) No storage of unencrypted kernel image to disk (no suspend-to-disk
>      without hardware support).
>
> I have patches pending that effect most of the above.  However, the
> firmware signature checking is being handled by someone else.  Further, it
> has come to light recently that debugfs needs attention, so that isn't done
> yet.
>
> Note that the secure boot mode entry doesn't currently work if the kernel
> is booted from current i386/x86_64 Grub as there's a bug in Grub whereby it
> doesn't initialise the boot_params correctly.  The incorrect initialisation
> causes sanitize_boot_params() to be triggered, thereby zapping the secure
> boot flag determined by the EFI boot wrapper.
>

Hello David,

By itself, this series looks in reasonable shape to me. But I do have
a few remaining concerns, apologies if it includes issues that I could
have brought up earlier.

- The series conflates 'UEFI secure boot support' with 'kernel lock
down support'. I think this has been brought up before, but I really
think we should have a cleaner separation between the feature (locking
down various bits of the kernel if lockdown is in effect) from the
policy 'enable lockdown if UEFI secure boot is enabled'. The latter
does not need to be configurable at all: on any UEFI system, we could
detect whether UEFI secure boot is in effect and report it. The only
tunable we need is in the lockdown context, whether lockdown needs to
take effect automatically when UEFI secure boot is detected (but there
could be other ways to enable lockdown, including a kernel cmdline
param or a sysfs node). Similarly, whether lockdown can be lifted or
not has *nothing* to do with whether it was enabled due to UEFI secure
boot, so I don't see the point of having
CONFIG_EFI_ALLOW_SECURE_BOOT_EXIT at all.

- The current series enables lockdown, but does not lock anything
down. Even if the code is in good shape otherwise, I am reluctant to
ack and/or merge anything right now, given that it provides a false
sense of security. This also ties in to my more general concerns with
this code (and I am aware I never replied to your email explaining it,
my apologies [again]): without any sense of how large the attack
surface is now, and how much we reduced it by implementing the items
on your list above, we should really not be making claims of security,
given that we really have no idea how much more secure we are. That
said, I do subscribe to the effort, in the sense that moving towards
the goal is strictly better than moving away from it, or not at all.

- Patch 5/5 breaks the build on non-x86. Please build test EFI-related
patches on arm64 and/or ARM before submitting patches. And if
possible, could we find a magic SysRq key that works on all
architectures? I know we discussed this at some point (I think?) but I
don't remember the conclusion.

Regards,
Ard.


> ---
> David Howells (3):
>       efi: Move the x86 secure boot switch to generic code
>       Add the ability to lock down access to the running kernel image
>       efi: Lock down the kernel if booted in secure boot mode
>
> Josh Boyer (1):
>       efi: Add EFI_SECURE_BOOT bit
>
> Kyle McMartin (1):
>       Add a sysrq option to exit secure boot mode
>
>
>  arch/x86/include/asm/efi.h        |    2 +
>  arch/x86/kernel/setup.c           |   14 ------
>  drivers/firmware/efi/Kconfig      |   34 ++++++++++++++++
>  drivers/firmware/efi/Makefile     |    1
>  drivers/firmware/efi/secureboot.c |   80 +++++++++++++++++++++++++++++++++++++
>  drivers/input/misc/uinput.c       |    1
>  drivers/tty/sysrq.c               |   19 ++++++---
>  include/linux/efi.h               |    7 +++
>  include/linux/input.h             |    5 ++
>  include/linux/kernel.h            |    9 ++++
>  include/linux/security.h          |   11 +++++
>  include/linux/sysrq.h             |    8 +++-
>  kernel/debug/kdb/kdb_main.c       |    2 -
>  security/Kconfig                  |   15 +++++++
>  security/Makefile                 |    3 +
>  security/lock_down.c              |   46 +++++++++++++++++++++
>  16 files changed, 236 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/firmware/efi/secureboot.c
>  create mode 100644 security/lock_down.c
>

[toc] | [next] | [standalone]

#1654050

Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> - The series conflates 'UEFI secure boot support' with 'kernel lock
> down support'. I think this has been brought up before, but I really
> think we should have a cleaner separation between the feature (locking
> down various bits of the kernel if lockdown is in effect) from the
> policy 'enable lockdown if UEFI secure boot is enabled'.

I'm not sure what you're actually asking for.  Are you wanting me to push the
lockdown patches upstream separately from the UEFI patches that trigger the
lockdown?  Or do you mean something else?

> The only tunable we need is in the lockdown context, whether lockdown needs
> to take effect automatically when UEFI secure boot is detected

So you don't want this change:

	config EFI_SECURE_BOOT
		bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
		default n
	+	select LOCK_DOWN_KERNEL

but would rather have a separate option asking whether lockdown should be
triggered if detect UEFI secure boot mode?

> (but there could be other ways to enable lockdown, including a kernel
> cmdline param or a sysfs node).

A sysfs node is pretty pointless.  A number of things that are locked down
need to be locked down during boot.  I could, however, provide a command-line
option to engage it or a kernel config option to unconditionally enable it.

> - The current series enables lockdown, but does not lock anything
> down.

Yes.  As I pointed out, and as I'm sure you know, I have a slew of other
patches that actually *do* lock things down.  I extracted these patches to try
and get some feedback on this bit without spamming various mailing lists with
all the other patches each time.

> Even if the code is in good shape otherwise, I am reluctant to
> ack and/or merge anything right now, given that it provides a false
> sense of security.

You're a member of the "make it provably 100% secure or nothing" camp?

> This also ties in to my more general concerns with this code (and I am aware
> I never replied to your email explaining it, my apologies [again]): without
> any sense of how large the attack surface is now,

I suspect no one knows.  I've been trying to lock down possibilities people
have pointed me at, even if some of them are very theoretical, and a lot of
the patches I've gathered together come from other people, but I don't know
the hardware that a lot of this is dealing with, so I can't answer this
question.

> and how much we reduced it by implementing the items on your list above, we
> should really not be making claims of security, given that we really have no
> idea how much more secure we are. That said, I do subscribe to the effort,
> in the sense that moving towards the goal is strictly better than moving
> away from it, or not at all.

Can we at least decide whether or not we want to put a locked-down mode into
the upstream kernel?  If not, I can drop this effort.

David

[toc] | [prev] | [next] | [standalone]

#1654143

On 31 May 2017 at 09:23, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> - The series conflates 'UEFI secure boot support' with 'kernel lock
>> down support'. I think this has been brought up before, but I really
>> think we should have a cleaner separation between the feature (locking
>> down various bits of the kernel if lockdown is in effect) from the
>> policy 'enable lockdown if UEFI secure boot is enabled'.
>
> I'm not sure what you're actually asking for.  Are you wanting me to push the
> lockdown patches upstream separately from the UEFI patches that trigger the
> lockdown?  Or do you mean something else?
>

No, I am fine with keeping this as a single series. I don't want
anything under drivers/efi to imply policy regarding lockdown. Kernel
lockdown should be a feature that lives somewhere else, and which
contains a CONFIG_ option that implies 'lockdown is enabled by default
when UEFI secure boot is detected.' The code that gets added to
drivers/efi should only concern itself with establishing whether
secure boot is in effect or not (and can hence be enabled
unconditionally)

>> The only tunable we need is in the lockdown context, whether lockdown needs
>> to take effect automatically when UEFI secure boot is detected
>
> So you don't want this change:
>
>         config EFI_SECURE_BOOT
>                 bool "Support UEFI Secure Boot and lock down the kernel in secure boot mode"
>                 default n
>         +       select LOCK_DOWN_KERNEL
>
> but would rather have a separate option asking whether lockdown should be
> triggered if detect UEFI secure boot mode?
>

Yes. As I said, supporting UEFI secure boot (whatever that could mean)
and implementing a lockdown policy when it is enabled are two entirely
different things.

>> (but there could be other ways to enable lockdown, including a kernel
>> cmdline param or a sysfs node).
>
> A sysfs node is pretty pointless.  A number of things that are locked down
> need to be locked down during boot.

OK.

> I could, however, provide a command-line
> option to engage it or a kernel config option to unconditionally enable it.
>

I *think* that could be useful, although it was meant as an
illustration rather than actual suggestion.

>> - The current series enables lockdown, but does not lock anything
>> down.
>
> Yes.  As I pointed out, and as I'm sure you know, I have a slew of other
> patches that actually *do* lock things down.  I extracted these patches to try
> and get some feedback on this bit without spamming various mailing lists with
> all the other patches each time.
>
>> Even if the code is in good shape otherwise, I am reluctant to
>> ack and/or merge anything right now, given that it provides a false
>> sense of security.
>
> You're a member of the "make it provably 100% secure or nothing" camp?
>

No, not at all. But patch 4/5 contains this line

pr_info("Secure boot enabled and kernel locked down\n");

which I don't like at all, given that nothing is actually locked down.
I have been working in this area long enough to know that people will
interpret 'lockdown' or 'secure' to mean anything they like if it is
left unqualified.

So what I would prefer is to separate this from the EFI code, and
perhaps print something like

lockdown: Kernel lockdown policy in effect due to xxx

and print a subsequent line for every lockdown feature that is enabled, e.g.,

lockdown: disabling MSRs
lockdown: disabling hibernate support

etc etc

These are just examples, of course, but it manages the expectations,
and as a bonus, it makes differences between architectures much more
visible.

>> This also ties in to my more general concerns with this code (and I am aware
>> I never replied to your email explaining it, my apologies [again]): without
>> any sense of how large the attack surface is now,
>
> I suspect no one knows.  I've been trying to lock down possibilities people
> have pointed me at, even if some of them are very theoretical, and a lot of
> the patches I've gathered together come from other people, but I don't know
> the hardware that a lot of this is dealing with, so I can't answer this
> question.
>

OK, and that is fine. I would just like this modest stance to be
reflected in the code and especially the log messages.

>> and how much we reduced it by implementing the items on your list above, we
>> should really not be making claims of security, given that we really have no
>> idea how much more secure we are. That said, I do subscribe to the effort,
>> in the sense that moving towards the goal is strictly better than moving
>> away from it, or not at all.
>
> Can we at least decide whether or not we want to put a locked-down mode into
> the upstream kernel?  If not, I can drop this effort.

No, I think this is a very important feature. But it needs to be more
transparent to avoid a false sense of security.

[toc] | [prev] | [next] | [standalone]

#1654232

Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> No, I am fine with keeping this as a single series. I don't want
> anything under drivers/efi to imply policy regarding lockdown. Kernel
> lockdown should be a feature that lives somewhere else, and which
> contains a CONFIG_ option that implies 'lockdown is enabled by default
> when UEFI secure boot is detected.' The code that gets added to
> drivers/efi should only concern itself with establishing whether
> secure boot is in effect or not (and can hence be enabled
> unconditionally)
> ...
> So what I would prefer is to separate this from the EFI code,

In that case I don't know where to connect the UEFI secure boot with the
lockdown code.

I was under the impression that you wanted the switch-statement that I had in
x86 setup.c moved to the efi code (as I've done in patch 1).  Was I wrong in
that assessment and that you actually wanted it, say, in security?

I don't think that the non-EFI core code should know about UEFI secure boot
mode.  Either the arch needs to implement the connection or the EFI code needs
to implement it.  In the former is preferred, I should drop patch 1.

> ... and perhaps print something like
> 
> lockdown: Kernel lockdown policy in effect due to xxx

I'm okay with printing that instead.

> and print a subsequent line for every lockdown feature that is enabled, e.g.,
> 
> lockdown: disabling MSRs
> lockdown: disabling hibernate support

That could add a lot of lines to the boot output:-/

David

[toc] | [prev] | [next] | [standalone]

#1654278

On 31 May 2017 at 13:33, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> No, I am fine with keeping this as a single series. I don't want
>> anything under drivers/efi to imply policy regarding lockdown. Kernel
>> lockdown should be a feature that lives somewhere else, and which
>> contains a CONFIG_ option that implies 'lockdown is enabled by default
>> when UEFI secure boot is detected.' The code that gets added to
>> drivers/efi should only concern itself with establishing whether
>> secure boot is in effect or not (and can hence be enabled
>> unconditionally)
>> ...
>> So what I would prefer is to separate this from the EFI code,
>
> In that case I don't know where to connect the UEFI secure boot with the
> lockdown code.
>
> I was under the impression that you wanted the switch-statement that I had in
> x86 setup.c moved to the efi code (as I've done in patch 1).  Was I wrong in
> that assessment and that you actually wanted it, say, in security?
>

No, that patch, and the patch that sets the EFI_SECURE_BOOT flag are
perfectly fine. I just think it should be the lockdown code that
contains the efi_enabled(EFI_SECURE_BOOT) check. Note that linux/efi.h
does the right thing in case CONFIG_EFI is not defined.

> I don't think that the non-EFI core code should know about UEFI secure boot
> mode.  Either the arch needs to implement the connection or the EFI code needs
> to implement it.  In the former is preferred, I should drop patch 1.
>
>> ... and perhaps print something like
>>
>> lockdown: Kernel lockdown policy in effect due to xxx
>
> I'm okay with printing that instead.
>
>> and print a subsequent line for every lockdown feature that is enabled, e.g.,
>>
>> lockdown: disabling MSRs
>> lockdown: disabling hibernate support
>
> That could add a lot of lines to the boot output:-/
>

Why is that a bad thing?

[toc] | [prev] | [next] | [standalone]

#1658532

Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

> and print a subsequent line for every lockdown feature that is enabled, e.g.,
> 
> lockdown: disabling MSRs
> lockdown: disabling hibernate support

There's another problem with this idea: the lockdown facility is passive - it
doesn't go looking for things to lock down; rather, things that can be locked
down inquire as to whether lockdown is in effect at the point someone tries to
use them.

Now, I could reserve a variable for each thing we lock down to make sure that
we don't emit the message more than once, but I'm loathe to waste memory this
way.

I can't so easily switch the facility to being active either, since a lot of
the lockdownables are in modules.

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web