Re: [PATCH] efi/libstub: arm: omit sorting of the UEFI memory map

[Matthias Brugger]

Hi Ard,

On 10/22/2017 04:14 PM, Ard Biesheuvel wrote:
> ARM shares its EFI stub implementation with arm64, which has some
> special handling in the virtual remapping code to
> a) make sure that we can map everything even if the OS executes
>with 64k page size, and
> b) make sure that adjacent regions with the same attributes are not
>reordered or moved apart in memory.
> 
> The latter is a workaround for a 'feature' that was shortly recommended
> by UEFI spec v2.5, but deprecated shortly after, due to the fact that
> it broke many OS installers, including non-Linux ones, and it was never
> widely implemented for ARM systems. Before implementing b), the arm64
> code simply rounded up all regions to 64 KB granularity, but given that
> that results in moving adjacent regions apart, it had to be refined when
> b) was implemented.
> 
> The adjacency check requires a sort() pass, due to the fact that the
> UEFI spec does not mandate any ordering, and the inclusion of the
> lib/sort.c code into the ARM EFI stub is causing some trouble with
> the decompressor build due to the fact that its EXPORT_SYMBOL() call
> triggers the creation of ksymtab/kcrctab sections.
> 
> So let's simply do away with the adjacency check for ARM, and simply put
> all UEFI runtime regions together if they have the same memory attributes.
> This is guaranteed to work, given that ARM only supports 4 KB pages,
> and allows us to remove the sort() call entirely.
> 
> Signed-off-by: Ard Biesheuvel 
> ---

Tested-by: Matthias Brugger 

This fixes my boot regression.
Thanks a lot!

>  drivers/firmware/efi/libstub/Makefile   | 6 +++---
>  drivers/firmware/efi/libstub/arm-stub.c | 7 +--
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/firmware/efi/libstub/Makefile 
> b/drivers/firmware/efi/libstub/Makefile
> index dedf9bde44db..f3e8431565ea 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -33,13 +33,14 @@ lib-y := efi-stub-helper.o 
> gop.o secureboot.o
>  lib-$(CONFIG_RESET_ATTACK_MITIGATION) += tpm.o
>  
>  # include the stub's generic dependencies from lib/ when building for 
> ARM/arm64
> -arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c 
> sort.c
> +arm-deps-y := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c
> +arm-deps-$(CONFIG_ARM64) += sort.c
>  
>  $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>   $(call if_changed_rule,cc_o_c)
>  
>  lib-$(CONFIG_EFI_ARMSTUB)+= arm-stub.o fdt.o string.o random.o \
> -$(patsubst %.c,lib-%.o,$(arm-deps))
> +$(patsubst %.c,lib-%.o,$(arm-deps-y))
>  
>  lib-$(CONFIG_ARM)+= arm32-stub.o
>  lib-$(CONFIG_ARM64)  += arm64-stub.o
> @@ -90,5 +91,4 @@ quiet_cmd_stubcopy = STUBCPY $@
>  # explicitly by the decompressor linker script.
>  #
>  STUBCOPY_FLAGS-$(CONFIG_ARM) += --rename-section .data=.data.efistub
> -STUBCOPY_RM-$(CONFIG_ARM)+= -R ___ksymtab+sort -R ___kcrctab+sort
>  STUBCOPY_RELOC-$(CONFIG_ARM) := R_ARM_ABS
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c 
> b/drivers/firmware/efi/libstub/arm-stub.c
> index 1cb2d1c070c3..3061e4057483 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -349,7 +349,9 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, 
> unsigned long map_size,
>* The easiest way to find adjacent regions is to sort the memory map
>* before traversing it.
>*/
> - sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc, NULL);
> + if (IS_ENABLED(CONFIG_ARM64))
> + sort(memory_map, map_size / desc_size, desc_size, cmp_mem_desc,
> +  NULL);
>  
>   for (l = 0; l < map_size; l += desc_size, prev = in) {
>   u64 paddr, size;
> @@ -366,7 +368,8 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, 
> unsigned long map_size,
>* a 4k page size kernel to kexec a 64k page size kernel and
>* vice versa.
>*/
> - if (!regions_are_adjacent(prev, in) ||
> + if ((IS_ENABLED(CONFIG_ARM64) &&
> +  !regions_are_adjacent(prev, in)) ||
>   !regions_have_compatible_memory_type_attrs(prev, in)) {
>  
>   paddr = round_down(in->phys_addr, SZ_64K);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/27] kexec_file: Disable at runtime if securelevel has been set

[Mimi Zohar]

On Thu, 2017-10-19 at 15:51 +0100, David Howells wrote:
> From: Chun-Yi Lee 
> 
> When KEXEC_VERIFY_SIG is not enabled, kernel should not loads image
> through kexec_file systemcall if securelevel has been set.

The patch title and description needs to be updated to refer to
lockdown, not securelevel.

As previously mentioned the last time these patches were posted, this
leaves out testing to see if the integrity subsystem is enabled.

Commit 503ceaef8e2e "ima: define a set of appraisal rules requiring
file signatures" was upstreamed.  An additional patch could force
these rules to be added to the custom policy, if lockdown is enabled.
 This and other patches in this series could then check to see if
is_ima_appraise_enabled() is true.

Mimi


> This code was showed in Matthew's patch but not in git:
> https://lkml.org/lkml/2015/3/13/778
> 
> Cc: Matthew Garrett 
> Signed-off-by: Chun-Yi Lee 
> Signed-off-by: David Howells 
> cc: ke...@lists.infradead.org
> ---
> 
>  kernel/kexec_file.c |7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> index 9f48f4412297..ff6523f2dcc2 100644
> --- a/kernel/kexec_file.c
> +++ b/kernel/kexec_file.c
> @@ -255,6 +255,13 @@ SYSCALL_DEFINE5(kexec_file_load, int, kernel_fd, int, 
> initrd_fd,
>   if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
>   return -EPERM;
> 
> + /* Don't permit images to be loaded into trusted kernels if we're not
> +  * going to verify the signature on them
> +  */
> + if (!IS_ENABLED(CONFIG_KEXEC_VERIFY_SIG) &&
> + kernel_is_locked_down("kexec of unsigned images"))
> + return -EPERM;
> +
>   /* Make sure we have a legal set of flags */
>   if (flags != (flags & KEXEC_FILE_FLAGS))
>   return -EINVAL;
> 
> --
> To unsubscribe from this list: send the line "unsubscribe 
> linux-security-module" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 26/27] efi: Add an EFI_SECURE_BOOT flag to indicate secure boot mode

[David Howells]

James Morris  wrote:

> > +   default:
> > +   pr_info("Secure boot could not be determined\n");
> 
> Perhaps make this pr_warning and include the unknown mode value?

Done.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down

[David Howells]

j...@suse.com wrote:

> hm... patch 4 only prevents write_mem() but not read_mem().
> Or I missed anything?

Actually, yes, as it happens, patch 11 prevents you from even opening /dev/mem
and /dev/kmem by locking down open of /dev/port.  So I've moved this bit to
patch 4, simplified and posted a new variant for patch 4.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/27] x86/msr: Restrict MSR access when the kernel is locked down

[David Howells]

Alan Cox  wrote:

> There are a load of standard tools that use this so I think you are going
> to need a whitelist. Can you at least log *which* MSR in the failing case
> so a whitelist can be built over time ?

Will the attached change work for you?

David
---
diff --git a/arch/x86/kernel/msr.c b/arch/x86/kernel/msr.c
index a05a97863286..f18cadbc31ce 100644
--- a/arch/x86/kernel/msr.c
+++ b/arch/x86/kernel/msr.c
@@ -84,8 +84,10 @@ static ssize_t msr_write(struct file *file, const char 
__user *buf,
int err = 0;
ssize_t bytes = 0;
 
-   if (kernel_is_locked_down("Direct MSR access"))
+   if (kernel_is_locked_down("Direct MSR access")) {
+   pr_info("Direct access to MSR %x\n", reg);
return -EPERM;
+   }
 
if (count % 8)
return -EINVAL; /* Invalid chunk size */
@@ -135,6 +137,7 @@ static long msr_ioctl(struct file *file, unsigned int ioc, 
unsigned long arg)
break;
}
if (kernel_is_locked_down("Direct MSR access")) {
+   pr_info("Direct access to MSR %x\n", reg[1]); /* 
Display %ecx */
err = -EPERM;
break;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/27] Restrict /dev/mem and /dev/kmem when the kernel is locked down

[David Howells]

I think I should replace this patch with the attached.  This will prevent
/dev/mem, /dev/kmem and /dev/port from being *opened*, and thereby preventing
read, write and ioctl.

David
---
commit e68daa2256986932b9a7d6709cf9e24b30d93583
Author: Matthew Garrett 
Date:   Wed May 24 14:56:02 2017 +0100

Restrict /dev/{mem,kmem,port} when the kernel is locked down

Allowing users to read and write to core kernel memory makes it possible
for the kernel to be subverted, avoiding module loading restrictions, and
also to steal cryptographic information.

Disallow /dev/mem and /dev/kmem from being opened this when the kernel has
been locked down to prevent this.

Also disallow /dev/port from being opened to prevent raw ioport access and
thus DMA from being used to accomplish the same thing.

Signed-off-by: Matthew Garrett 
Signed-off-by: David Howells 
Reviewed-by: "Lee, Chun-Yi" 

diff --git a/drivers/char/mem.c b/drivers/char/mem.c
index 593a8818aca9..0ce5ac0a5c6b 100644
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -762,6 +762,8 @@ static loff_t memory_lseek(struct file *file, loff_t 
offset, int orig)
 
 static int open_port(struct inode *inode, struct file *filp)
 {
+   if (kernel_is_locked_down("/dev/mem,kmem,port"))
+   return -EPERM;
return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] efi/libstub: arm: don't randomize runtime regions when CONFIG_HIBERNATION=y

[Ard Biesheuvel]

Commit

  e69176d68d26 ef/libstub/arm/arm64: Randomize the base of the UEFI rt services 
region

implemented randomization of the virtual mapping that the OS chooses for
the UEFI runtime services. This was motivated by the fact that UEFI usually
does not bother to specify any permission restrictions for those regions,
making them prime real estate for exploitation now that the OS is getting
more and more careful not to leave any R+W+X mapped regions lying around.

However, this randomization breaks assumptions in the resume from
hibernation code, which expects all memory regions populated by UEFI to
remain in the same place, including their virtual mapping into the OS
memory space. While this assumption may not be entirely reasonable in the
first place, breaking it deliberately does not make a lot of sense either.
So let's refrain from this randomization pass if CONFIG_HIBERNATION=y.

Cc: James Morse 
Cc: Matt Fleming 
Signed-off-by: Ard Biesheuvel 
---
 drivers/firmware/efi/libstub/arm-stub.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/arm-stub.c 
b/drivers/firmware/efi/libstub/arm-stub.c
index 3061e4057483..01a9d78ee415 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -238,7 +238,8 @@ unsigned long efi_entry(void *handle, efi_system_table_t 
*sys_table,
 
efi_random_get_seed(sys_table);
 
-   if (!nokaslr()) {
+   /* hibernation expects the runtime regions to stay in the same place */
+   if (!IS_ENABLED(CONFIG_HIBERNATION) && !nokaslr()) {
/*
 * Randomize the base of the UEFI runtime services region.
 * Preserve the 2 MB alignment of the region by taking a
-- 
2.11.0

--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html