[PATCH 0/5] EFI Secure boot support

[PATCH 0/5] EFI Secure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	This patch serie is the first one to add the secure boot support
	to barebox on efi

	For now on this will allow you to execute only properly signed EFI
	Application.

	And request confirmation for non signed binary other than EFI

	Later will add non signed EFI Application execution request at user
	request and MoK Support, add other goodies.

Please pull
The following changes since commit 071eb906506d478a3d0b04460625e64edbcd1294:

  efi: fix secure and setup mode report (2017-03-04 13:20:38 +0800)

are available in the git repository at:

  git://git.jcrosoft.org/barebox.git delivery/efi-sb

for you to fetch changes up to 99e61a9be467fab5b5127b1233c4c4e70288e84c:

  efi: enable sercure boot support (2017-03-04 13:20:38 +0800)

----------------------------------------------------------------
Jean-Christophe PLAGNIOL-VILLARD (2):
      boot: if we are in secure boot mode
      efi: enable sercure boot support

 arch/x86/Kconfig         |  1 +
 commands/go.c            |  7 +++++++
 common/Kconfig           |  8 ++++++++
 common/Makefile          |  1 +
 common/bootm.c           |  7 +++++++
 common/efi/efi-image.c   |  1 +
 common/secure_boot.c     | 43 +++++++++++++++++++++++++++++++++++++++++++
 drivers/efi/efi-device.c |  9 +++++++++
 include/bootm.h          |  1 +
 include/secure_boot.h    | 25 +++++++++++++++++++++++++
 10 files changed, 103 insertions(+)
 create mode 100644 common/secure_boot.c
 create mode 100644 include/secure_boot.h

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/5] efi: add more security related guid for the efivars

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 common/efi-guid.c |  6 ++++++
 include/efi.h     | 18 ++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/common/efi-guid.c b/common/efi-guid.c
index 71aa21ddd..01b02bbb1 100644
--- a/common/efi-guid.c
+++ b/common/efi-guid.c
@@ -81,6 +81,12 @@ const char *efi_guid_string(efi_guid_t *g)
 	EFI_GUID_STRING( EFI_TCG_PROTOCOL_GUID, "TcgService", "TCGServices Protocol");
 	/* TPM 2.0 */
 	EFI_GUID_STRING( EFI_TCG2_PROTOCOL_GUID, "Tcg2Service", "TCG2Services Protocol");
+	EFI_GUID_STRING(EFI_VENDOR_KEYS_NV_VARIABLE_NAME_GUID, "VendorKeysNv", "Vendor Keys Non-Volatile");
+	EFI_GUID_STRING(EFI_AUTHENTICATED_VARIABLE_GUID, "AuthVar", "Authenticated Variable");
+	EFI_GUID_STRING(EFI_IMAGE_SECURITY_DATABASE_GUID, "ImageSecurityDB", "Image Security Database");
+	EFI_GUID_STRING(EFI_CERT_DB_GUID, "CertDB", "Certificate Database");
+	EFI_GUID_STRING(EFI_SECURE_BOOT_ENABLE_DISABLE_GUID, "SecureBootEnable", "Secure Boot Enable Disable");
+	EFI_GUID_STRING(EFI_CUSTOM_MODE_ENABLE_GUID, "CustomMode", "CustomMode");
 
 	/* File */
 	EFI_GUID_STRING(EFI_IDEBUSDXE_INF_GUID, "IdeBusDxe.inf", "EFI IdeBusDxe.inf File GUID");
diff --git a/include/efi.h b/include/efi.h
index e1fc134ee..31e7b283e 100644
--- a/include/efi.h
+++ b/include/efi.h
@@ -507,6 +507,24 @@ extern efi_runtime_services_t *RT;
 #define EFI_TCG2_PROTOCOL_GUID \
 	EFI_GUID(0x607f766c, 0x7455, 0x42be, 0x93, 0x0b, 0xe4, 0xd7, 0x6d, 0xb2, 0x72, 0x0f)
 
+#define EFI_VENDOR_KEYS_NV_VARIABLE_NAME_GUID \
+	EFI_GUID(0x9073e4e0, 0x60ec, 0x4b6e, 0x99, 0x3, 0x4c, 0x22, 0x3c, 0x26, 0x0f, 0x3c)
+
+#define EFI_AUTHENTICATED_VARIABLE_GUID \
+	EFI_GUID(0xaaf32c78, 0x947b, 0x439a, 0xa1, 0x80, 0x2e, 0x14, 0x4e, 0xc3,0x77, 0x92)
+
+#define EFI_IMAGE_SECURITY_DATABASE_GUID \
+	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0xe, 0x67, 0x65, 0x6f)
+
+#define EFI_CERT_DB_GUID \
+	EFI_GUID(0xd9bee56e, 0x75dc, 0x49d9, 0xb4, 0xd7, 0xb5, 0x34, 0x21, 0xf, 0x63, 0x7a)
+
+#define EFI_SECURE_BOOT_ENABLE_DISABLE_GUID \
+	EFI_GUID(0xf0a30bc7, 0xaf08, 0x4556, 0x99, 0xc4, 0x00, 0x10, 0x9, 0xc9, 0x3a, 0x44)
+
+#define EFI_CUSTOM_MODE_ENABLE_GUID \
+	EFI_GUID(0xc076ec0c, 0x7028, 0x4399, 0xa0, 0x72, 0x71, 0xee, 0x5c, 0x44, 0x8b, 0x9f)
+
 extern efi_guid_t efi_file_info_id;
 extern efi_guid_t efi_simple_file_system_protocol_guid;
 extern efi_guid_t efi_device_path_protocol_guid;
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/5] efi: fix lds for secure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

everythink need to be aligned to 4096

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 arch/x86/mach-efi/elf_ia32_efi.lds.S         | 10 +++++++---
 arch/x86/mach-efi/elf_x86_64_efi.lds.S       | 10 ++++++----
 arch/x86/mach-efi/include/mach/barebox.lds.h | 14 +++++++++++++-
 include/asm-generic/barebox.lds.h            |  8 +++++---
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/arch/x86/mach-efi/elf_ia32_efi.lds.S b/arch/x86/mach-efi/elf_ia32_efi.lds.S
index 69f43f554..6d9cb973c 100644
--- a/arch/x86/mach-efi/elf_ia32_efi.lds.S
+++ b/arch/x86/mach-efi/elf_ia32_efi.lds.S
@@ -50,22 +50,23 @@ SECTIONS
 		*(COMMON)
 	}
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 
 	__barebox_initcalls_start = .;
 	__barebox_initcalls : { INITCALLS }
 	__barebox_initcalls_end = .;
+	. = ALIGN(4096);
 
 	__barebox_exitcalls_start = .;
 	__barebox_exitcalls : { EXITCALLS }
 	__barebox_exitcalls_end = .;
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 	__barebox_magicvar_start = .;
 	.barebox_magicvar : { BAREBOX_MAGICVARS }
 	__barebox_magicvar_end = .;
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 	__barebox_cmd_start = .;
 	__barebox_cmd : { BAREBOX_CMDS }
 	__barebox_cmd_end = .;
@@ -76,6 +77,9 @@ SECTIONS
 	.rel : {
 		*(.rel.data)
 		*(.rel.data.*)
+		*(.rela.barebox*)
+		*(.rela.initcall*)
+		*(.rela.exitcall*)
 		*(.rel.got)
 		*(.rel.stab)
 		*(.data.rel.ro.local)
diff --git a/arch/x86/mach-efi/elf_x86_64_efi.lds.S b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
index 93d34d17a..8216d1d70 100644
--- a/arch/x86/mach-efi/elf_x86_64_efi.lds.S
+++ b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
@@ -23,6 +23,7 @@ SECTIONS
 		*(.text)
 		*(.text.*)
 		*(.gnu.linkonce.t.*)
+		. = ALIGN(16);
 	}
 
 	_etext = .;
@@ -33,8 +34,8 @@ SECTIONS
 		*(.reloc)
 	}
 
-	. = ALIGN(4096);
 	_sdata = .;
+	. = ALIGN(4096);
 
 	.data : {
 		*(.rodata*)
@@ -52,22 +53,23 @@ SECTIONS
 		*(.rel.local)
 	}
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 
 	__barebox_initcalls_start = .;
 	__barebox_initcalls : { INITCALLS }
 	__barebox_initcalls_end = .;
+	. = ALIGN(4096);
 
 	__barebox_exitcalls_start = .;
 	__barebox_exitcalls : { EXITCALLS }
 	__barebox_exitcalls_end = .;
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 	__barebox_magicvar_start = .;
 	.barebox_magicvar : { BAREBOX_MAGICVARS }
 	__barebox_magicvar_end = .;
 
-	. = ALIGN(64);
+	. = ALIGN(4096);
 	__barebox_cmd_start = .;
 	__barebox_cmd : { BAREBOX_CMDS }
 	__barebox_cmd_end = .;
diff --git a/arch/x86/mach-efi/include/mach/barebox.lds.h b/arch/x86/mach-efi/include/mach/barebox.lds.h
index 40a8c178f..e7a3bb9cd 100644
--- a/arch/x86/mach-efi/include/mach/barebox.lds.h
+++ b/arch/x86/mach-efi/include/mach/barebox.lds.h
@@ -1 +1,13 @@
-/* empty */
+/*
+ * Copyright (C) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnio@jcrosoft.com>
+ *
+ * Under GPL v2
+ */
+
+#ifndef __EFI_MACH_BAREBOX_LDS_H__
+#define __EFI_MACH_BAREBOX_LDS_H__
+
+/* For secure boot we need all the section to be 4096 alligned */
+#define STRUCT_ALIGNMENT 4096
+
+#endif /* __EFI_MACH_BAREBOX_LDS_H__ */
diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
index c8a919b92..6c37751b3 100644
--- a/include/asm-generic/barebox.lds.h
+++ b/include/asm-generic/barebox.lds.h
@@ -3,15 +3,17 @@
  * Align to a 32 byte boundary equal to the
  * alignment gcc 4.5 uses for a struct
  */
-#define STRUCT_ALIGNMENT 32
-#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
-
 #if defined CONFIG_X86 || \
 	defined CONFIG_ARCH_EP93XX || \
 	defined CONFIG_ARCH_ZYNQ
 #include <mach/barebox.lds.h>
 #endif
 
+#ifndef STRUCT_ALIGNMENT
+#define STRUCT_ALIGNMENT 32
+#endif
+#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
+
 #ifndef PRE_IMAGE
 #define PRE_IMAGE
 #endif
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/5] efi: fix secure and setup mode report

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 drivers/efi/efi-device.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/efi/efi-device.c b/drivers/efi/efi-device.c
index 6ed7f12b3..7029bfb31 100644
--- a/drivers/efi/efi-device.c
+++ b/drivers/efi/efi-device.c
@@ -365,7 +365,7 @@ static int efi_is_secure_boot(void)
 		free(val);
 	}
 
-	return ret != 1;
+	return ret != 0;
 }
 
 static int efi_is_setup_mode(void)
@@ -379,7 +379,7 @@ static int efi_is_setup_mode(void)
 		free(val);
 	}
 
-	return ret != 1;
+	return ret != 0;
 }
 
 static int efi_init_devices(void)
@@ -406,7 +406,7 @@ static int efi_init_devices(void)
 	dev_add_param_int_ro(efi_bus.dev, "fw_revision", efi_sys_table->fw_revision, "%u");
 	dev_add_param_int_ro(efi_bus.dev, "secure_boot", secure_boot, "%d");
 	dev_add_param_int_ro(efi_bus.dev, "secure_mode",
-			     secure_boot & setup_mode, "%u");
+			     secure_boot & !setup_mode, "%u");
 
 	efi_bus.dev->info = efi_businfo;
 
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 4/5] boot: if we are in secure boot mode

[Jean-Christophe PLAGNIOL-VILLARD]

request confirmation before booting an unsigned image

with a default timeout

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 commands/go.c         |  7 +++++++
 common/Kconfig        |  8 ++++++++
 common/Makefile       |  1 +
 common/bootm.c        |  7 +++++++
 common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
 include/bootm.h       |  1 +
 include/secure_boot.h | 25 +++++++++++++++++++++++++
 7 files changed, 92 insertions(+)
 create mode 100644 common/secure_boot.c
 create mode 100644 include/secure_boot.h

diff --git a/commands/go.c b/commands/go.c
index fb319b320..61c9ce43f 100644
--- a/commands/go.c
+++ b/commands/go.c
@@ -26,6 +26,7 @@
 #include <fcntl.h>
 #include <linux/ctype.h>
 #include <errno.h>
+#include <secure_boot.h>
 
 static int do_go(int argc, char *argv[])
 {
@@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
 	if (argc < 2)
 		return COMMAND_ERROR_USAGE;
 
+	rcode = secure_boot_start_unsigned();
+	if (rcode)
+		return rcode ? 1 : 0;
+
+	rcode = 1;
+
 	if (!isdigit(*argv[1])) {
 		fd = open(argv[1], O_RDONLY);
 		if (fd < 0) {
diff --git a/common/Kconfig b/common/Kconfig
index f7ff04664..9e7d027a2 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -21,6 +21,9 @@ config HAS_KALLSYMS
 config HAS_MODULES
 	bool
 
+config HAS_SECURE_BOOT
+	bool
+
 config HAS_CACHE
 	bool
 	help
@@ -184,6 +187,11 @@ config NVVAR
 	  while global variables can be changed during runtime without changing the
 	  default.
 
+config SECURE_BOOT_TIMEOUT
+	prompt "Secure Boot unsigned boot timeout"
+	int
+	default 3
+
 menu "memory layout"
 
 source "pbl/Kconfig"
diff --git a/common/Makefile b/common/Makefile
index 5f58c81d2..e57cc2c15 100644
--- a/common/Makefile
+++ b/common/Makefile
@@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
 obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
 obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
 obj-$(CONFIG_BOOT)		+= boot.o
+obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
 
 quiet_cmd_pwd_h = PWDH    $@
 ifdef CONFIG_PASSWORD
diff --git a/common/bootm.c b/common/bootm.c
index 81625d915..0ebf65681 100644
--- a/common/bootm.c
+++ b/common/bootm.c
@@ -23,6 +23,7 @@
 #include <environment.h>
 #include <linux/stat.h>
 #include <magicvar.h>
+#include <secure_boot.h>
 
 static LIST_HEAD(handler_list);
 
@@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
 		printf("Passing control to %s handler\n", handler->name);
 	}
 
+	if (!handler->is_secure_supported && is_secure_mode()) {
+		ret = secure_boot_start_unsigned();
+		if (ret)
+			goto err_out;
+	}
+
 	ret = handler->bootm(data);
 	if (data->dryrun)
 		printf("Dryrun. Aborted\n");
diff --git a/common/secure_boot.c b/common/secure_boot.c
new file mode 100644
index 000000000..e625ef4e1
--- /dev/null
+++ b/common/secure_boot.c
@@ -0,0 +1,43 @@
+/*
+ * Copyright (c) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
+ *
+ * Under GPLv2 Only
+ */
+#include <common.h>
+#include <secure_boot.h>
+#include <console_countdown.h>
+#include <globalvar.h>
+#include <magicvar.h>
+#include <init.h>
+
+static unsigned int sb_confirm_timeout = CONFIG_SECURE_BOOT_TIMEOUT;
+
+int secure_boot_start_unsigned(void)
+{
+	int ret;
+	char str[2] = { };
+	int timeout = sb_confirm_timeout;
+
+	if (!is_secure_mode())
+		return 0;
+
+	printf("Are you sure you wish to run an unsigned binary\n");
+	printf("in a secure environment?\n");
+	printf("press y to confirm\n");
+
+	ret = console_countdown(timeout, CONSOLE_COUNTDOWN_ANYKEY, str);
+	if (ret != -EINTR)
+		return -EINVAL;
+
+	return str[0] == 'y' ? 0 : -EINVAL;
+}
+
+static int init_sb_confirm_timeout(void)
+{
+	return globalvar_add_simple_int("sb.confirm_timeout",
+			&sb_confirm_timeout, "%u");
+}
+late_initcall(init_sb_confirm_timeout);
+
+BAREBOX_MAGICVAR_NAMED(global_sb_confirm_timeout, global.sb.confirm_timeout,
+		"Secure Boot Comfirm timeout in seconds before booting an unsigned image");
diff --git a/include/bootm.h b/include/bootm.h
index 6e9777a9a..12a61c007 100644
--- a/include/bootm.h
+++ b/include/bootm.h
@@ -89,6 +89,7 @@ struct image_handler {
 	struct list_head list;
 
 	int ih_os;
+	int is_secure_supported;
 
 	enum filetype filetype;
 	int (*bootm)(struct image_data *data);
diff --git a/include/secure_boot.h b/include/secure_boot.h
new file mode 100644
index 000000000..3aa071012
--- /dev/null
+++ b/include/secure_boot.h
@@ -0,0 +1,25 @@
+/*
+ * Copyright (c) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
+ *
+ * Under GPLv2 Only
+ */
+
+#ifndef __SECURE_BOOT_H__
+#define __SECURE_BOOT_H__
+
+#ifdef CONFIG_HAS_SECURE_BOOT
+int is_secure_mode(void);
+int secure_boot_start_unsigned(void);
+#else
+static int inline is_secure_mode(void)
+{
+	return 0;
+}
+
+static int inline secure_boot_start_unsigned(void)
+{
+	return 0;
+}
+#endif
+
+#endif /* __SECURE_BOOT_H__ */
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 5/5] efi: enable sercure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

This will ensure that we just start secured binary
without user confirmation

But for now on we only support EFI correctly signed image to start
Later will allow both.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
---
 arch/x86/Kconfig         | 1 +
 common/efi/efi-image.c   | 1 +
 drivers/efi/efi-device.c | 9 +++++++++
 3 files changed, 11 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 52ccf4894..65e4c8b7c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -78,6 +78,7 @@ choice
 		select EFI_DEVICEPATH
 		select PRINTF_UUID
 		select CLOCKSOURCE_EFI_X86
+		select HAS_SECURE_BOOT
 
 	config X86_BIOS_BRINGUP
 		bool "16 bit BIOS"
diff --git a/common/efi/efi-image.c b/common/efi/efi-image.c
index 885348da4..6552d803d 100644
--- a/common/efi/efi-image.c
+++ b/common/efi/efi-image.c
@@ -270,6 +270,7 @@ static int do_bootm_efi(struct image_data *data)
 static struct image_handler efi_handle_tr = {
 	.name = "EFI Application",
 	.bootm = do_bootm_efi,
+	.is_secure_supported = 1,
 	.filetype = filetype_exe,
 };
 
diff --git a/drivers/efi/efi-device.c b/drivers/efi/efi-device.c
index 7029bfb31..959878e7f 100644
--- a/drivers/efi/efi-device.c
+++ b/drivers/efi/efi-device.c
@@ -26,6 +26,7 @@
 #include <linux/sizes.h>
 #include <wchar.h>
 #include <init.h>
+#include <secure_boot.h>
 #include <efi.h>
 #include <efi/efi.h>
 #include <efi/efi-device.h>
@@ -382,6 +383,14 @@ static int efi_is_setup_mode(void)
 	return ret != 0;
 }
 
+int is_secure_mode(void)
+{
+	int secure_boot = efi_is_secure_boot();
+	int setup_mode = efi_is_setup_mode();
+
+	return secure_boot & !setup_mode;
+}
+
 static int efi_init_devices(void)
 {
 	char *fw_vendor = NULL;
-- 
2.11.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Lucas Stach]

Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
PLAGNIOL-VILLARD:
> everythink need to be aligned to 4096

Why? The commit message isn't really telling anything.

Regards,
Lucas 

> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  arch/x86/mach-efi/elf_ia32_efi.lds.S         | 10 +++++++---
>  arch/x86/mach-efi/elf_x86_64_efi.lds.S       | 10 ++++++----
>  arch/x86/mach-efi/include/mach/barebox.lds.h | 14 +++++++++++++-
>  include/asm-generic/barebox.lds.h            |  8 +++++---
>  4 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/arch/x86/mach-efi/elf_ia32_efi.lds.S b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> index 69f43f554..6d9cb973c 100644
> --- a/arch/x86/mach-efi/elf_ia32_efi.lds.S
> +++ b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> @@ -50,22 +50,23 @@ SECTIONS
>  		*(COMMON)
>  	}
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  
>  	__barebox_initcalls_start = .;
>  	__barebox_initcalls : { INITCALLS }
>  	__barebox_initcalls_end = .;
> +	. = ALIGN(4096);
>  
>  	__barebox_exitcalls_start = .;
>  	__barebox_exitcalls : { EXITCALLS }
>  	__barebox_exitcalls_end = .;
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  	__barebox_magicvar_start = .;
>  	.barebox_magicvar : { BAREBOX_MAGICVARS }
>  	__barebox_magicvar_end = .;
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  	__barebox_cmd_start = .;
>  	__barebox_cmd : { BAREBOX_CMDS }
>  	__barebox_cmd_end = .;
> @@ -76,6 +77,9 @@ SECTIONS
>  	.rel : {
>  		*(.rel.data)
>  		*(.rel.data.*)
> +		*(.rela.barebox*)
> +		*(.rela.initcall*)
> +		*(.rela.exitcall*)
>  		*(.rel.got)
>  		*(.rel.stab)
>  		*(.data.rel.ro.local)
> diff --git a/arch/x86/mach-efi/elf_x86_64_efi.lds.S b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> index 93d34d17a..8216d1d70 100644
> --- a/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> +++ b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> @@ -23,6 +23,7 @@ SECTIONS
>  		*(.text)
>  		*(.text.*)
>  		*(.gnu.linkonce.t.*)
> +		. = ALIGN(16);
>  	}
>  
>  	_etext = .;
> @@ -33,8 +34,8 @@ SECTIONS
>  		*(.reloc)
>  	}
>  
> -	. = ALIGN(4096);
>  	_sdata = .;
> +	. = ALIGN(4096);
>  
>  	.data : {
>  		*(.rodata*)
> @@ -52,22 +53,23 @@ SECTIONS
>  		*(.rel.local)
>  	}
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  
>  	__barebox_initcalls_start = .;
>  	__barebox_initcalls : { INITCALLS }
>  	__barebox_initcalls_end = .;
> +	. = ALIGN(4096);
>  
>  	__barebox_exitcalls_start = .;
>  	__barebox_exitcalls : { EXITCALLS }
>  	__barebox_exitcalls_end = .;
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  	__barebox_magicvar_start = .;
>  	.barebox_magicvar : { BAREBOX_MAGICVARS }
>  	__barebox_magicvar_end = .;
>  
> -	. = ALIGN(64);
> +	. = ALIGN(4096);
>  	__barebox_cmd_start = .;
>  	__barebox_cmd : { BAREBOX_CMDS }
>  	__barebox_cmd_end = .;
> diff --git a/arch/x86/mach-efi/include/mach/barebox.lds.h b/arch/x86/mach-efi/include/mach/barebox.lds.h
> index 40a8c178f..e7a3bb9cd 100644
> --- a/arch/x86/mach-efi/include/mach/barebox.lds.h
> +++ b/arch/x86/mach-efi/include/mach/barebox.lds.h
> @@ -1 +1,13 @@
> -/* empty */
> +/*
> + * Copyright (C) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnio@jcrosoft.com>
> + *
> + * Under GPL v2
> + */
> +
> +#ifndef __EFI_MACH_BAREBOX_LDS_H__
> +#define __EFI_MACH_BAREBOX_LDS_H__
> +
> +/* For secure boot we need all the section to be 4096 alligned */
> +#define STRUCT_ALIGNMENT 4096
> +
> +#endif /* __EFI_MACH_BAREBOX_LDS_H__ */
> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> index c8a919b92..6c37751b3 100644
> --- a/include/asm-generic/barebox.lds.h
> +++ b/include/asm-generic/barebox.lds.h
> @@ -3,15 +3,17 @@
>   * Align to a 32 byte boundary equal to the
>   * alignment gcc 4.5 uses for a struct
>   */
> -#define STRUCT_ALIGNMENT 32
> -#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> -
>  #if defined CONFIG_X86 || \
>  	defined CONFIG_ARCH_EP93XX || \
>  	defined CONFIG_ARCH_ZYNQ
>  #include <mach/barebox.lds.h>
>  #endif
>  
> +#ifndef STRUCT_ALIGNMENT
> +#define STRUCT_ALIGNMENT 32
> +#endif
> +#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> +
>  #ifndef PRE_IMAGE
>  #define PRE_IMAGE
>  #endif



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

> On Mar 10, 2017, at 1:24 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> 
> Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
> PLAGNIOL-VILLARD:
>> everythink need to be aligned to 4096
> 
> Why? The commit message isn't really telling anything.
This is a requierment by EFI

Best Regards,
J.
> 
> Regards,
> Lucas 
> 
>> 
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> ---
>> arch/x86/mach-efi/elf_ia32_efi.lds.S         | 10 +++++++---
>> arch/x86/mach-efi/elf_x86_64_efi.lds.S       | 10 ++++++----
>> arch/x86/mach-efi/include/mach/barebox.lds.h | 14 +++++++++++++-
>> include/asm-generic/barebox.lds.h            |  8 +++++---
>> 4 files changed, 31 insertions(+), 11 deletions(-)
>> 
>> diff --git a/arch/x86/mach-efi/elf_ia32_efi.lds.S b/arch/x86/mach-efi/elf_ia32_efi.lds.S
>> index 69f43f554..6d9cb973c 100644
>> --- a/arch/x86/mach-efi/elf_ia32_efi.lds.S
>> +++ b/arch/x86/mach-efi/elf_ia32_efi.lds.S
>> @@ -50,22 +50,23 @@ SECTIONS
>> 		*(COMMON)
>> 	}
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 
>> 	__barebox_initcalls_start = .;
>> 	__barebox_initcalls : { INITCALLS }
>> 	__barebox_initcalls_end = .;
>> +	. = ALIGN(4096);
>> 
>> 	__barebox_exitcalls_start = .;
>> 	__barebox_exitcalls : { EXITCALLS }
>> 	__barebox_exitcalls_end = .;
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 	__barebox_magicvar_start = .;
>> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
>> 	__barebox_magicvar_end = .;
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 	__barebox_cmd_start = .;
>> 	__barebox_cmd : { BAREBOX_CMDS }
>> 	__barebox_cmd_end = .;
>> @@ -76,6 +77,9 @@ SECTIONS
>> 	.rel : {
>> 		*(.rel.data)
>> 		*(.rel.data.*)
>> +		*(.rela.barebox*)
>> +		*(.rela.initcall*)
>> +		*(.rela.exitcall*)
>> 		*(.rel.got)
>> 		*(.rel.stab)
>> 		*(.data.rel.ro.local)
>> diff --git a/arch/x86/mach-efi/elf_x86_64_efi.lds.S b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
>> index 93d34d17a..8216d1d70 100644
>> --- a/arch/x86/mach-efi/elf_x86_64_efi.lds.S
>> +++ b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
>> @@ -23,6 +23,7 @@ SECTIONS
>> 		*(.text)
>> 		*(.text.*)
>> 		*(.gnu.linkonce.t.*)
>> +		. = ALIGN(16);
>> 	}
>> 
>> 	_etext = .;
>> @@ -33,8 +34,8 @@ SECTIONS
>> 		*(.reloc)
>> 	}
>> 
>> -	. = ALIGN(4096);
>> 	_sdata = .;
>> +	. = ALIGN(4096);
>> 
>> 	.data : {
>> 		*(.rodata*)
>> @@ -52,22 +53,23 @@ SECTIONS
>> 		*(.rel.local)
>> 	}
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 
>> 	__barebox_initcalls_start = .;
>> 	__barebox_initcalls : { INITCALLS }
>> 	__barebox_initcalls_end = .;
>> +	. = ALIGN(4096);
>> 
>> 	__barebox_exitcalls_start = .;
>> 	__barebox_exitcalls : { EXITCALLS }
>> 	__barebox_exitcalls_end = .;
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 	__barebox_magicvar_start = .;
>> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
>> 	__barebox_magicvar_end = .;
>> 
>> -	. = ALIGN(64);
>> +	. = ALIGN(4096);
>> 	__barebox_cmd_start = .;
>> 	__barebox_cmd : { BAREBOX_CMDS }
>> 	__barebox_cmd_end = .;
>> diff --git a/arch/x86/mach-efi/include/mach/barebox.lds.h b/arch/x86/mach-efi/include/mach/barebox.lds.h
>> index 40a8c178f..e7a3bb9cd 100644
>> --- a/arch/x86/mach-efi/include/mach/barebox.lds.h
>> +++ b/arch/x86/mach-efi/include/mach/barebox.lds.h
>> @@ -1 +1,13 @@
>> -/* empty */
>> +/*
>> + * Copyright (C) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnio@jcrosoft.com>
>> + *
>> + * Under GPL v2
>> + */
>> +
>> +#ifndef __EFI_MACH_BAREBOX_LDS_H__
>> +#define __EFI_MACH_BAREBOX_LDS_H__
>> +
>> +/* For secure boot we need all the section to be 4096 alligned */
>> +#define STRUCT_ALIGNMENT 4096
>> +
>> +#endif /* __EFI_MACH_BAREBOX_LDS_H__ */
>> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
>> index c8a919b92..6c37751b3 100644
>> --- a/include/asm-generic/barebox.lds.h
>> +++ b/include/asm-generic/barebox.lds.h
>> @@ -3,15 +3,17 @@
>>  * Align to a 32 byte boundary equal to the
>>  * alignment gcc 4.5 uses for a struct
>>  */
>> -#define STRUCT_ALIGNMENT 32
>> -#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
>> -
>> #if defined CONFIG_X86 || \
>> 	defined CONFIG_ARCH_EP93XX || \
>> 	defined CONFIG_ARCH_ZYNQ
>> #include <mach/barebox.lds.h>
>> #endif
>> 
>> +#ifndef STRUCT_ALIGNMENT
>> +#define STRUCT_ALIGNMENT 32
>> +#endif
>> +#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
>> +
>> #ifndef PRE_IMAGE
>> #define PRE_IMAGE
>> #endif
> 
> 


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Lucas Stach]

Am Freitag, den 10.03.2017, 18:17 +0800 schrieb Jean-Christophe
PLAGNIOL-VILLARD:
> > On Mar 10, 2017, at 1:24 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > 
> > Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
> > PLAGNIOL-VILLARD:
> >> everythink need to be aligned to 4096
> > 
> > Why? The commit message isn't really telling anything.
> This is a requierment by EFI

This is in no way an EFI requirement.

Googling tells me that the signing procedure for EFI secure boot is
built around a PE binary. PE in turn is based on the COFF binary format
which, unlike ELF, has no section descriptions in the header and
therefore requires the sections to be placed page aligned (4K on x86, 4K
or 64K on ARM64).

All of the above is what should have been included in the commit
message, to allow other people to understand the commit and not require
them to google their way to the justification of this commit.

Regards,
Lucas

> Best Regards,
> J.
> > 
> > Regards,
> > Lucas 
> > 
> >> 
> >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> >> ---
> >> arch/x86/mach-efi/elf_ia32_efi.lds.S         | 10 +++++++---
> >> arch/x86/mach-efi/elf_x86_64_efi.lds.S       | 10 ++++++----
> >> arch/x86/mach-efi/include/mach/barebox.lds.h | 14 +++++++++++++-
> >> include/asm-generic/barebox.lds.h            |  8 +++++---
> >> 4 files changed, 31 insertions(+), 11 deletions(-)
> >> 
> >> diff --git a/arch/x86/mach-efi/elf_ia32_efi.lds.S b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> >> index 69f43f554..6d9cb973c 100644
> >> --- a/arch/x86/mach-efi/elf_ia32_efi.lds.S
> >> +++ b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> >> @@ -50,22 +50,23 @@ SECTIONS
> >> 		*(COMMON)
> >> 	}
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 
> >> 	__barebox_initcalls_start = .;
> >> 	__barebox_initcalls : { INITCALLS }
> >> 	__barebox_initcalls_end = .;
> >> +	. = ALIGN(4096);
> >> 
> >> 	__barebox_exitcalls_start = .;
> >> 	__barebox_exitcalls : { EXITCALLS }
> >> 	__barebox_exitcalls_end = .;
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 	__barebox_magicvar_start = .;
> >> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
> >> 	__barebox_magicvar_end = .;
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 	__barebox_cmd_start = .;
> >> 	__barebox_cmd : { BAREBOX_CMDS }
> >> 	__barebox_cmd_end = .;
> >> @@ -76,6 +77,9 @@ SECTIONS
> >> 	.rel : {
> >> 		*(.rel.data)
> >> 		*(.rel.data.*)
> >> +		*(.rela.barebox*)
> >> +		*(.rela.initcall*)
> >> +		*(.rela.exitcall*)
> >> 		*(.rel.got)
> >> 		*(.rel.stab)
> >> 		*(.data.rel.ro.local)
> >> diff --git a/arch/x86/mach-efi/elf_x86_64_efi.lds.S b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> >> index 93d34d17a..8216d1d70 100644
> >> --- a/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> >> +++ b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> >> @@ -23,6 +23,7 @@ SECTIONS
> >> 		*(.text)
> >> 		*(.text.*)
> >> 		*(.gnu.linkonce.t.*)
> >> +		. = ALIGN(16);
> >> 	}
> >> 
> >> 	_etext = .;
> >> @@ -33,8 +34,8 @@ SECTIONS
> >> 		*(.reloc)
> >> 	}
> >> 
> >> -	. = ALIGN(4096);
> >> 	_sdata = .;
> >> +	. = ALIGN(4096);
> >> 
> >> 	.data : {
> >> 		*(.rodata*)
> >> @@ -52,22 +53,23 @@ SECTIONS
> >> 		*(.rel.local)
> >> 	}
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 
> >> 	__barebox_initcalls_start = .;
> >> 	__barebox_initcalls : { INITCALLS }
> >> 	__barebox_initcalls_end = .;
> >> +	. = ALIGN(4096);
> >> 
> >> 	__barebox_exitcalls_start = .;
> >> 	__barebox_exitcalls : { EXITCALLS }
> >> 	__barebox_exitcalls_end = .;
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 	__barebox_magicvar_start = .;
> >> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
> >> 	__barebox_magicvar_end = .;
> >> 
> >> -	. = ALIGN(64);
> >> +	. = ALIGN(4096);
> >> 	__barebox_cmd_start = .;
> >> 	__barebox_cmd : { BAREBOX_CMDS }
> >> 	__barebox_cmd_end = .;
> >> diff --git a/arch/x86/mach-efi/include/mach/barebox.lds.h b/arch/x86/mach-efi/include/mach/barebox.lds.h
> >> index 40a8c178f..e7a3bb9cd 100644
> >> --- a/arch/x86/mach-efi/include/mach/barebox.lds.h
> >> +++ b/arch/x86/mach-efi/include/mach/barebox.lds.h
> >> @@ -1 +1,13 @@
> >> -/* empty */
> >> +/*
> >> + * Copyright (C) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnio@jcrosoft.com>
> >> + *
> >> + * Under GPL v2
> >> + */
> >> +
> >> +#ifndef __EFI_MACH_BAREBOX_LDS_H__
> >> +#define __EFI_MACH_BAREBOX_LDS_H__
> >> +
> >> +/* For secure boot we need all the section to be 4096 alligned */
> >> +#define STRUCT_ALIGNMENT 4096
> >> +
> >> +#endif /* __EFI_MACH_BAREBOX_LDS_H__ */
> >> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> >> index c8a919b92..6c37751b3 100644
> >> --- a/include/asm-generic/barebox.lds.h
> >> +++ b/include/asm-generic/barebox.lds.h
> >> @@ -3,15 +3,17 @@
> >>  * Align to a 32 byte boundary equal to the
> >>  * alignment gcc 4.5 uses for a struct
> >>  */
> >> -#define STRUCT_ALIGNMENT 32
> >> -#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> >> -
> >> #if defined CONFIG_X86 || \
> >> 	defined CONFIG_ARCH_EP93XX || \
> >> 	defined CONFIG_ARCH_ZYNQ
> >> #include <mach/barebox.lds.h>
> >> #endif
> >> 
> >> +#ifndef STRUCT_ALIGNMENT
> >> +#define STRUCT_ALIGNMENT 32
> >> +#endif
> >> +#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> >> +
> >> #ifndef PRE_IMAGE
> >> #define PRE_IMAGE
> >> #endif
> > 
> > 
> 



_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:05 Fri 10 Mar     , Lucas Stach wrote:
> Am Freitag, den 10.03.2017, 18:17 +0800 schrieb Jean-Christophe
> PLAGNIOL-VILLARD:
> > > On Mar 10, 2017, at 1:24 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > 
> > > Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
> > > PLAGNIOL-VILLARD:
> > >> everythink need to be aligned to 4096
> > > 
> > > Why? The commit message isn't really telling anything.
> > This is a requierment by EFI
> 
> This is in no way an EFI requirement.
> 
> Googling tells me that the signing procedure for EFI secure boot is
> built around a PE binary. PE in turn is based on the COFF binary format
> which, unlike ELF, has no section descriptions in the header and
> therefore requires the sections to be placed page aligned (4K on x86, 4K
> or 64K on ARM64).
No COFF does not require to have section "page aligned" which is wrong here
The is a Requirement by EFI from secure boot

Otherwise EFI will not work today

> 
> All of the above is what should have been included in the commit
> message, to allow other people to understand the commit and not require
> them to google their way to the justification of this commit.
And read the Spec is required to undrestant EFI no?
Or I need to put the spec in the commit too?

Best Regards,
J
> 
> Regards,
> Lucas
> 
> > Best Regards,
> > J.
> > > 
> > > Regards,
> > > Lucas 
> > > 
> > >> 
> > >> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > >> ---
> > >> arch/x86/mach-efi/elf_ia32_efi.lds.S         | 10 +++++++---
> > >> arch/x86/mach-efi/elf_x86_64_efi.lds.S       | 10 ++++++----
> > >> arch/x86/mach-efi/include/mach/barebox.lds.h | 14 +++++++++++++-
> > >> include/asm-generic/barebox.lds.h            |  8 +++++---
> > >> 4 files changed, 31 insertions(+), 11 deletions(-)
> > >> 
> > >> diff --git a/arch/x86/mach-efi/elf_ia32_efi.lds.S b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> > >> index 69f43f554..6d9cb973c 100644
> > >> --- a/arch/x86/mach-efi/elf_ia32_efi.lds.S
> > >> +++ b/arch/x86/mach-efi/elf_ia32_efi.lds.S
> > >> @@ -50,22 +50,23 @@ SECTIONS
> > >> 		*(COMMON)
> > >> 	}
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 
> > >> 	__barebox_initcalls_start = .;
> > >> 	__barebox_initcalls : { INITCALLS }
> > >> 	__barebox_initcalls_end = .;
> > >> +	. = ALIGN(4096);
> > >> 
> > >> 	__barebox_exitcalls_start = .;
> > >> 	__barebox_exitcalls : { EXITCALLS }
> > >> 	__barebox_exitcalls_end = .;
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 	__barebox_magicvar_start = .;
> > >> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
> > >> 	__barebox_magicvar_end = .;
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 	__barebox_cmd_start = .;
> > >> 	__barebox_cmd : { BAREBOX_CMDS }
> > >> 	__barebox_cmd_end = .;
> > >> @@ -76,6 +77,9 @@ SECTIONS
> > >> 	.rel : {
> > >> 		*(.rel.data)
> > >> 		*(.rel.data.*)
> > >> +		*(.rela.barebox*)
> > >> +		*(.rela.initcall*)
> > >> +		*(.rela.exitcall*)
> > >> 		*(.rel.got)
> > >> 		*(.rel.stab)
> > >> 		*(.data.rel.ro.local)
> > >> diff --git a/arch/x86/mach-efi/elf_x86_64_efi.lds.S b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> > >> index 93d34d17a..8216d1d70 100644
> > >> --- a/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> > >> +++ b/arch/x86/mach-efi/elf_x86_64_efi.lds.S
> > >> @@ -23,6 +23,7 @@ SECTIONS
> > >> 		*(.text)
> > >> 		*(.text.*)
> > >> 		*(.gnu.linkonce.t.*)
> > >> +		. = ALIGN(16);
> > >> 	}
> > >> 
> > >> 	_etext = .;
> > >> @@ -33,8 +34,8 @@ SECTIONS
> > >> 		*(.reloc)
> > >> 	}
> > >> 
> > >> -	. = ALIGN(4096);
> > >> 	_sdata = .;
> > >> +	. = ALIGN(4096);
> > >> 
> > >> 	.data : {
> > >> 		*(.rodata*)
> > >> @@ -52,22 +53,23 @@ SECTIONS
> > >> 		*(.rel.local)
> > >> 	}
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 
> > >> 	__barebox_initcalls_start = .;
> > >> 	__barebox_initcalls : { INITCALLS }
> > >> 	__barebox_initcalls_end = .;
> > >> +	. = ALIGN(4096);
> > >> 
> > >> 	__barebox_exitcalls_start = .;
> > >> 	__barebox_exitcalls : { EXITCALLS }
> > >> 	__barebox_exitcalls_end = .;
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 	__barebox_magicvar_start = .;
> > >> 	.barebox_magicvar : { BAREBOX_MAGICVARS }
> > >> 	__barebox_magicvar_end = .;
> > >> 
> > >> -	. = ALIGN(64);
> > >> +	. = ALIGN(4096);
> > >> 	__barebox_cmd_start = .;
> > >> 	__barebox_cmd : { BAREBOX_CMDS }
> > >> 	__barebox_cmd_end = .;
> > >> diff --git a/arch/x86/mach-efi/include/mach/barebox.lds.h b/arch/x86/mach-efi/include/mach/barebox.lds.h
> > >> index 40a8c178f..e7a3bb9cd 100644
> > >> --- a/arch/x86/mach-efi/include/mach/barebox.lds.h
> > >> +++ b/arch/x86/mach-efi/include/mach/barebox.lds.h
> > >> @@ -1 +1,13 @@
> > >> -/* empty */
> > >> +/*
> > >> + * Copyright (C) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnio@jcrosoft.com>
> > >> + *
> > >> + * Under GPL v2
> > >> + */
> > >> +
> > >> +#ifndef __EFI_MACH_BAREBOX_LDS_H__
> > >> +#define __EFI_MACH_BAREBOX_LDS_H__
> > >> +
> > >> +/* For secure boot we need all the section to be 4096 alligned */
> > >> +#define STRUCT_ALIGNMENT 4096
> > >> +
> > >> +#endif /* __EFI_MACH_BAREBOX_LDS_H__ */
> > >> diff --git a/include/asm-generic/barebox.lds.h b/include/asm-generic/barebox.lds.h
> > >> index c8a919b92..6c37751b3 100644
> > >> --- a/include/asm-generic/barebox.lds.h
> > >> +++ b/include/asm-generic/barebox.lds.h
> > >> @@ -3,15 +3,17 @@
> > >>  * Align to a 32 byte boundary equal to the
> > >>  * alignment gcc 4.5 uses for a struct
> > >>  */
> > >> -#define STRUCT_ALIGNMENT 32
> > >> -#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> > >> -
> > >> #if defined CONFIG_X86 || \
> > >> 	defined CONFIG_ARCH_EP93XX || \
> > >> 	defined CONFIG_ARCH_ZYNQ
> > >> #include <mach/barebox.lds.h>
> > >> #endif
> > >> 
> > >> +#ifndef STRUCT_ALIGNMENT
> > >> +#define STRUCT_ALIGNMENT 32
> > >> +#endif
> > >> +#define STRUCT_ALIGN() . = ALIGN(STRUCT_ALIGNMENT)
> > >> +
> > >> #ifndef PRE_IMAGE
> > >> #define PRE_IMAGE
> > >> #endif
> > > 
> > > 
> > 
> 
> 

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Lucas Stach]

Am Freitag, den 10.03.2017, 14:54 +0100 schrieb Jean-Christophe
PLAGNIOL-VILLARD:
> On 12:05 Fri 10 Mar     , Lucas Stach wrote:
> > Am Freitag, den 10.03.2017, 18:17 +0800 schrieb Jean-Christophe
> > PLAGNIOL-VILLARD:
> > > > On Mar 10, 2017, at 1:24 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
> > > > PLAGNIOL-VILLARD:
> > > >> everythink need to be aligned to 4096
> > > > 
> > > > Why? The commit message isn't really telling anything.
> > > This is a requierment by EFI
> > 
> > This is in no way an EFI requirement.
> > 
> > Googling tells me that the signing procedure for EFI secure boot is
> > built around a PE binary. PE in turn is based on the COFF binary format
> > which, unlike ELF, has no section descriptions in the header and
> > therefore requires the sections to be placed page aligned (4K on x86, 4K
> > or 64K on ARM64).
> No COFF does not require to have section "page aligned" which is wrong here
> The is a Requirement by EFI from secure boot
> 
> Otherwise EFI will not work today
> 
> > 
> > All of the above is what should have been included in the commit
> > message, to allow other people to understand the commit and not require
> > them to google their way to the justification of this commit.
> And read the Spec is required to undrestant EFI no?
> Or I need to put the spec in the commit too?
> 
At least a pointer to the relevant part of the spec would be
appreciated.

That we are even having this conversation shows that your commit
messages are a bit too terse. Being a bit more verbose would be really
helpful for other folks trying to understand your changes.

Thanks,
Lucas


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 2/5] efi: fix lds for secure boot support

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:54 Fri 10 Mar     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:05 Fri 10 Mar     , Lucas Stach wrote:
> > Am Freitag, den 10.03.2017, 18:17 +0800 schrieb Jean-Christophe
> > PLAGNIOL-VILLARD:
> > > > On Mar 10, 2017, at 1:24 AM, Lucas Stach <l.stach@pengutronix.de> wrote:
> > > > 
> > > > Am Donnerstag, den 09.03.2017, 15:34 +0100 schrieb Jean-Christophe
> > > > PLAGNIOL-VILLARD:
> > > >> everythink need to be aligned to 4096
> > > > 
> > > > Why? The commit message isn't really telling anything.
> > > This is a requierment by EFI
> > 
> > This is in no way an EFI requirement.
> > 
> > Googling tells me that the signing procedure for EFI secure boot is
> > built around a PE binary. PE in turn is based on the COFF binary format
> > which, unlike ELF, has no section descriptions in the header and
> > therefore requires the sections to be placed page aligned (4K on x86, 4K
> > or 64K on ARM64).
> No COFF does not require to have section "page aligned" which is wrong here

This is the job of the loader to aligned them in memory but on EFI it's
required to sign the binary.

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/5] efi: fix secure and setup mode report

[Sascha Hauer]

On Thu, Mar 09, 2017 at 03:34:08PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  drivers/efi/efi-device.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/efi/efi-device.c b/drivers/efi/efi-device.c
> index 6ed7f12b3..7029bfb31 100644
> --- a/drivers/efi/efi-device.c
> +++ b/drivers/efi/efi-device.c
> @@ -365,7 +365,7 @@ static int efi_is_secure_boot(void)
>  		free(val);
>  	}
>  
> -	return ret != 1;
> +	return ret != 0;
>  }
>  
>  static int efi_is_setup_mode(void)
> @@ -379,7 +379,7 @@ static int efi_is_setup_mode(void)
>  		free(val);
>  	}
>  
> -	return ret != 1;
> +	return ret != 0;
>  }
>  
>  static int efi_init_devices(void)
> @@ -406,7 +406,7 @@ static int efi_init_devices(void)
>  	dev_add_param_int_ro(efi_bus.dev, "fw_revision", efi_sys_table->fw_revision, "%u");
>  	dev_add_param_int_ro(efi_bus.dev, "secure_boot", secure_boot, "%d");
>  	dev_add_param_int_ro(efi_bus.dev, "secure_mode",
> -			     secure_boot & setup_mode, "%u");
> +			     secure_boot & !setup_mode, "%u");

While a bitwise 'and' operator surely works with booleans here you should still
use a logical 'and' operator here.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] boot: if we are in secure boot mode

[Sascha Hauer]

On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> request confirmation before booting an unsigned image
> 
> with a default timeout
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  commands/go.c         |  7 +++++++
>  common/Kconfig        |  8 ++++++++
>  common/Makefile       |  1 +
>  common/bootm.c        |  7 +++++++
>  common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/bootm.h       |  1 +
>  include/secure_boot.h | 25 +++++++++++++++++++++++++
>  7 files changed, 92 insertions(+)
>  create mode 100644 common/secure_boot.c
>  create mode 100644 include/secure_boot.h
> 
> diff --git a/commands/go.c b/commands/go.c
> index fb319b320..61c9ce43f 100644
> --- a/commands/go.c
> +++ b/commands/go.c
> @@ -26,6 +26,7 @@
>  #include <fcntl.h>
>  #include <linux/ctype.h>
>  #include <errno.h>
> +#include <secure_boot.h>
>  
>  static int do_go(int argc, char *argv[])
>  {
> @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
>  	if (argc < 2)
>  		return COMMAND_ERROR_USAGE;
>  
> +	rcode = secure_boot_start_unsigned();
> +	if (rcode)
> +		return rcode ? 1 : 0;

When rcode is true, then if rcode is true, return 1, otherwise return 0?

> +
> +	rcode = 1;
> +
>  	if (!isdigit(*argv[1])) {
>  		fd = open(argv[1], O_RDONLY);
>  		if (fd < 0) {
> diff --git a/common/Kconfig b/common/Kconfig
> index f7ff04664..9e7d027a2 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -21,6 +21,9 @@ config HAS_KALLSYMS
>  config HAS_MODULES
>  	bool
>  
> +config HAS_SECURE_BOOT
> +	bool
> +
>  config HAS_CACHE
>  	bool
>  	help
> @@ -184,6 +187,11 @@ config NVVAR
>  	  while global variables can be changed during runtime without changing the
>  	  default.
>  
> +config SECURE_BOOT_TIMEOUT
> +	prompt "Secure Boot unsigned boot timeout"
> +	int
> +	default 3
> +
>  menu "memory layout"
>  
>  source "pbl/Kconfig"
> diff --git a/common/Makefile b/common/Makefile
> index 5f58c81d2..e57cc2c15 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
>  obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
>  obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
>  obj-$(CONFIG_BOOT)		+= boot.o
> +obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
>  
>  quiet_cmd_pwd_h = PWDH    $@
>  ifdef CONFIG_PASSWORD
> diff --git a/common/bootm.c b/common/bootm.c
> index 81625d915..0ebf65681 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -23,6 +23,7 @@
>  #include <environment.h>
>  #include <linux/stat.h>
>  #include <magicvar.h>
> +#include <secure_boot.h>
>  
>  static LIST_HEAD(handler_list);
>  
> @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
>  		printf("Passing control to %s handler\n", handler->name);
>  	}
>  
> +	if (!handler->is_secure_supported && is_secure_mode()) {
> +		ret = secure_boot_start_unsigned();
> +		if (ret)
> +			goto err_out;
> +	}
> +
>  	ret = handler->bootm(data);
>  	if (data->dryrun)
>  		printf("Dryrun. Aborted\n");
> diff --git a/common/secure_boot.c b/common/secure_boot.c
> new file mode 100644
> index 000000000..e625ef4e1
> --- /dev/null
> +++ b/common/secure_boot.c
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (c) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> + *
> + * Under GPLv2 Only
> + */
> +#include <common.h>
> +#include <secure_boot.h>
> +#include <console_countdown.h>
> +#include <globalvar.h>
> +#include <magicvar.h>
> +#include <init.h>
> +
> +static unsigned int sb_confirm_timeout = CONFIG_SECURE_BOOT_TIMEOUT;

Use globalvar_add_simple_int instead of putting this into the config.

> +
> +int secure_boot_start_unsigned(void)

This function name is very misleading. It sounds like it starts an
unsigned image, but this function doesn't start anything. Second guess
is that it returns a boolean whether it's allowed to start an unsigned
image, but actually it returns the opposite.

> +{
> +	int ret;
> +	char str[2] = { };
> +	int timeout = sb_confirm_timeout;
> +
> +	if (!is_secure_mode())
> +		return 0;
> +
> +	printf("Are you sure you wish to run an unsigned binary\n");
> +	printf("in a secure environment?\n");
> +	printf("press y to confirm\n");
> +
> +	ret = console_countdown(timeout, CONSOLE_COUNTDOWN_ANYKEY, str);

console_countdown puts exactly one character in *str, no need to pass an
array.

> +	if (ret != -EINTR)
> +		return -EINVAL;
> +
> +	return str[0] == 'y' ? 0 : -EINVAL;
> +}
> +
> +static int init_sb_confirm_timeout(void)
> +{
> +	return globalvar_add_simple_int("sb.confirm_timeout",
> +			&sb_confirm_timeout, "%u");
> +}

Ok, you already made it a globalvar. No need to put the default into
kconfig then.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] boot: if we are in secure boot mode

[Sascha Hauer]

On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> request confirmation before booting an unsigned image
> 
> with a default timeout
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> ---
>  commands/go.c         |  7 +++++++
>  common/Kconfig        |  8 ++++++++
>  common/Makefile       |  1 +
>  common/bootm.c        |  7 +++++++
>  common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/bootm.h       |  1 +
>  include/secure_boot.h | 25 +++++++++++++++++++++++++
>  7 files changed, 92 insertions(+)
>  create mode 100644 common/secure_boot.c
>  create mode 100644 include/secure_boot.h
> 
> diff --git a/commands/go.c b/commands/go.c
> index fb319b320..61c9ce43f 100644
> --- a/commands/go.c
> +++ b/commands/go.c
> @@ -26,6 +26,7 @@
>  #include <fcntl.h>
>  #include <linux/ctype.h>
>  #include <errno.h>
> +#include <secure_boot.h>
>  
>  static int do_go(int argc, char *argv[])
>  {
> @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
>  	if (argc < 2)
>  		return COMMAND_ERROR_USAGE;
>  
> +	rcode = secure_boot_start_unsigned();
> +	if (rcode)
> +		return rcode ? 1 : 0;
> +
> +	rcode = 1;
> +
>  	if (!isdigit(*argv[1])) {
>  		fd = open(argv[1], O_RDONLY);
>  		if (fd < 0) {
> diff --git a/common/Kconfig b/common/Kconfig
> index f7ff04664..9e7d027a2 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -21,6 +21,9 @@ config HAS_KALLSYMS
>  config HAS_MODULES
>  	bool
>  
> +config HAS_SECURE_BOOT
> +	bool
> +
>  config HAS_CACHE
>  	bool
>  	help
> @@ -184,6 +187,11 @@ config NVVAR
>  	  while global variables can be changed during runtime without changing the
>  	  default.
>  
> +config SECURE_BOOT_TIMEOUT
> +	prompt "Secure Boot unsigned boot timeout"
> +	int
> +	default 3
> +
>  menu "memory layout"
>  
>  source "pbl/Kconfig"
> diff --git a/common/Makefile b/common/Makefile
> index 5f58c81d2..e57cc2c15 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
>  obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
>  obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
>  obj-$(CONFIG_BOOT)		+= boot.o
> +obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
>  
>  quiet_cmd_pwd_h = PWDH    $@
>  ifdef CONFIG_PASSWORD
> diff --git a/common/bootm.c b/common/bootm.c
> index 81625d915..0ebf65681 100644
> --- a/common/bootm.c
> +++ b/common/bootm.c
> @@ -23,6 +23,7 @@
>  #include <environment.h>
>  #include <linux/stat.h>
>  #include <magicvar.h>
> +#include <secure_boot.h>
>  
>  static LIST_HEAD(handler_list);
>  
> @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
>  		printf("Passing control to %s handler\n", handler->name);
>  	}
>  
> +	if (!handler->is_secure_supported && is_secure_mode()) {
> +		ret = secure_boot_start_unsigned();
> +		if (ret)
> +			goto err_out;
> +	}

We already have CONFIG_BOOTM_FORCE_SIGNED_IMAGES which admittedly is not
the most generic implementation, but in the end we shouldn't have two
different ways how secure boot is integrated in the bootm code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] boot: if we are in secure boot mode

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:55 Mon 13 Mar     , Sascha Hauer wrote:
> On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > request confirmation before booting an unsigned image
> > 
> > with a default timeout
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  commands/go.c         |  7 +++++++
> >  common/Kconfig        |  8 ++++++++
> >  common/Makefile       |  1 +
> >  common/bootm.c        |  7 +++++++
> >  common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  include/bootm.h       |  1 +
> >  include/secure_boot.h | 25 +++++++++++++++++++++++++
> >  7 files changed, 92 insertions(+)
> >  create mode 100644 common/secure_boot.c
> >  create mode 100644 include/secure_boot.h
> > 
> > diff --git a/commands/go.c b/commands/go.c
> > index fb319b320..61c9ce43f 100644
> > --- a/commands/go.c
> > +++ b/commands/go.c
> > @@ -26,6 +26,7 @@
> >  #include <fcntl.h>
> >  #include <linux/ctype.h>
> >  #include <errno.h>
> > +#include <secure_boot.h>
> >  
> >  static int do_go(int argc, char *argv[])
> >  {
> > @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
> >  	if (argc < 2)
> >  		return COMMAND_ERROR_USAGE;
> >  
> > +	rcode = secure_boot_start_unsigned();
> > +	if (rcode)
> > +		return rcode ? 1 : 0;
> > +
> > +	rcode = 1;
> > +
> >  	if (!isdigit(*argv[1])) {
> >  		fd = open(argv[1], O_RDONLY);
> >  		if (fd < 0) {
> > diff --git a/common/Kconfig b/common/Kconfig
> > index f7ff04664..9e7d027a2 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -21,6 +21,9 @@ config HAS_KALLSYMS
> >  config HAS_MODULES
> >  	bool
> >  
> > +config HAS_SECURE_BOOT
> > +	bool
> > +
> >  config HAS_CACHE
> >  	bool
> >  	help
> > @@ -184,6 +187,11 @@ config NVVAR
> >  	  while global variables can be changed during runtime without changing the
> >  	  default.
> >  
> > +config SECURE_BOOT_TIMEOUT
> > +	prompt "Secure Boot unsigned boot timeout"
> > +	int
> > +	default 3
> > +
> >  menu "memory layout"
> >  
> >  source "pbl/Kconfig"
> > diff --git a/common/Makefile b/common/Makefile
> > index 5f58c81d2..e57cc2c15 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
> >  obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
> >  obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
> >  obj-$(CONFIG_BOOT)		+= boot.o
> > +obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
> >  
> >  quiet_cmd_pwd_h = PWDH    $@
> >  ifdef CONFIG_PASSWORD
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 81625d915..0ebf65681 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -23,6 +23,7 @@
> >  #include <environment.h>
> >  #include <linux/stat.h>
> >  #include <magicvar.h>
> > +#include <secure_boot.h>
> >  
> >  static LIST_HEAD(handler_list);
> >  
> > @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
> >  		printf("Passing control to %s handler\n", handler->name);
> >  	}
> >  
> > +	if (!handler->is_secure_supported && is_secure_mode()) {
> > +		ret = secure_boot_start_unsigned();
> > +		if (ret)
> > +			goto err_out;
> > +	}
> 
> We already have CONFIG_BOOTM_FORCE_SIGNED_IMAGES which admittedly is not
> the most generic implementation, but in the end we shouldn't have two
> different ways how secure boot is integrated in the bootm code.
Did not seen it

I check it and this is only for BOOTM as we have go command too

that need to call image check too

and on efi this is not done at config time but runtime

and this is really FIT image oriented :(

Can we have one for EFI sperately first and then merge them as this will need
a lot of testing and checking to ensure we break nothing

Best Regards,
J.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] boot: if we are in secure boot mode

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:50 Mon 13 Mar     , Sascha Hauer wrote:
> On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > request confirmation before booting an unsigned image
> > 
> > with a default timeout
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  commands/go.c         |  7 +++++++
> >  common/Kconfig        |  8 ++++++++
> >  common/Makefile       |  1 +
> >  common/bootm.c        |  7 +++++++
> >  common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  include/bootm.h       |  1 +
> >  include/secure_boot.h | 25 +++++++++++++++++++++++++
> >  7 files changed, 92 insertions(+)
> >  create mode 100644 common/secure_boot.c
> >  create mode 100644 include/secure_boot.h
> > 
> > diff --git a/commands/go.c b/commands/go.c
> > index fb319b320..61c9ce43f 100644
> > --- a/commands/go.c
> > +++ b/commands/go.c
> > @@ -26,6 +26,7 @@
> >  #include <fcntl.h>
> >  #include <linux/ctype.h>
> >  #include <errno.h>
> > +#include <secure_boot.h>
> >  
> >  static int do_go(int argc, char *argv[])
> >  {
> > @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
> >  	if (argc < 2)
> >  		return COMMAND_ERROR_USAGE;
> >  
> > +	rcode = secure_boot_start_unsigned();
> > +	if (rcode)
> > +		return rcode ? 1 : 0;
> 
> When rcode is true, then if rcode is true, return 1, otherwise return 0?

I forget we could return a negative error on command too
> 
> > +
> > +	rcode = 1;
> > +
> >  	if (!isdigit(*argv[1])) {
> >  		fd = open(argv[1], O_RDONLY);
> >  		if (fd < 0) {
> > diff --git a/common/Kconfig b/common/Kconfig
> > index f7ff04664..9e7d027a2 100644
> > --- a/common/Kconfig
> > +++ b/common/Kconfig
> > @@ -21,6 +21,9 @@ config HAS_KALLSYMS
> >  config HAS_MODULES
> >  	bool
> >  
> > +config HAS_SECURE_BOOT
> > +	bool
> > +
> >  config HAS_CACHE
> >  	bool
> >  	help
> > @@ -184,6 +187,11 @@ config NVVAR
> >  	  while global variables can be changed during runtime without changing the
> >  	  default.
> >  
> > +config SECURE_BOOT_TIMEOUT
> > +	prompt "Secure Boot unsigned boot timeout"
> > +	int
> > +	default 3
> > +
> >  menu "memory layout"
> >  
> >  source "pbl/Kconfig"
> > diff --git a/common/Makefile b/common/Makefile
> > index 5f58c81d2..e57cc2c15 100644
> > --- a/common/Makefile
> > +++ b/common/Makefile
> > @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
> >  obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
> >  obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
> >  obj-$(CONFIG_BOOT)		+= boot.o
> > +obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
> >  
> >  quiet_cmd_pwd_h = PWDH    $@
> >  ifdef CONFIG_PASSWORD
> > diff --git a/common/bootm.c b/common/bootm.c
> > index 81625d915..0ebf65681 100644
> > --- a/common/bootm.c
> > +++ b/common/bootm.c
> > @@ -23,6 +23,7 @@
> >  #include <environment.h>
> >  #include <linux/stat.h>
> >  #include <magicvar.h>
> > +#include <secure_boot.h>
> >  
> >  static LIST_HEAD(handler_list);
> >  
> > @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
> >  		printf("Passing control to %s handler\n", handler->name);
> >  	}
> >  
> > +	if (!handler->is_secure_supported && is_secure_mode()) {
> > +		ret = secure_boot_start_unsigned();
> > +		if (ret)
> > +			goto err_out;
> > +	}
> > +
> >  	ret = handler->bootm(data);
> >  	if (data->dryrun)
> >  		printf("Dryrun. Aborted\n");
> > diff --git a/common/secure_boot.c b/common/secure_boot.c
> > new file mode 100644
> > index 000000000..e625ef4e1
> > --- /dev/null
> > +++ b/common/secure_boot.c
> > @@ -0,0 +1,43 @@
> > +/*
> > + * Copyright (c) 2017 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > + *
> > + * Under GPLv2 Only
> > + */
> > +#include <common.h>
> > +#include <secure_boot.h>
> > +#include <console_countdown.h>
> > +#include <globalvar.h>
> > +#include <magicvar.h>
> > +#include <init.h>
> > +
> > +static unsigned int sb_confirm_timeout = CONFIG_SECURE_BOOT_TIMEOUT;
> 
> Use globalvar_add_simple_int instead of putting this into the config.
> 
> > +
> > +int secure_boot_start_unsigned(void)
> 
> This function name is very misleading. It sounds like it starts an
> unsigned image, but this function doesn't start anything. Second guess
> is that it returns a boolean whether it's allowed to start an unsigned
> image, but actually it returns the opposite.

secure_boot_can_start_unsigned could be better

it return 0 if ok
an error otherwise

as we can return an security violation or other error

Best Regards,
J.

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 3/5] efi: fix secure and setup mode report

[Jean-Christophe PLAGNIOL-VILLARD]

On 08:34 Mon 13 Mar     , Sascha Hauer wrote:
> On Thu, Mar 09, 2017 at 03:34:08PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > ---
> >  drivers/efi/efi-device.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/efi/efi-device.c b/drivers/efi/efi-device.c
> > index 6ed7f12b3..7029bfb31 100644
> > --- a/drivers/efi/efi-device.c
> > +++ b/drivers/efi/efi-device.c
> > @@ -365,7 +365,7 @@ static int efi_is_secure_boot(void)
> >  		free(val);
> >  	}
> >  
> > -	return ret != 1;
> > +	return ret != 0;
> >  }
> >  
> >  static int efi_is_setup_mode(void)
> > @@ -379,7 +379,7 @@ static int efi_is_setup_mode(void)
> >  		free(val);
> >  	}
> >  
> > -	return ret != 1;
> > +	return ret != 0;
> >  }
> >  
> >  static int efi_init_devices(void)
> > @@ -406,7 +406,7 @@ static int efi_init_devices(void)
> >  	dev_add_param_int_ro(efi_bus.dev, "fw_revision", efi_sys_table->fw_revision, "%u");
> >  	dev_add_param_int_ro(efi_bus.dev, "secure_boot", secure_boot, "%d");
> >  	dev_add_param_int_ro(efi_bus.dev, "secure_mode",
> > -			     secure_boot & setup_mode, "%u");
> > +			     secure_boot & !setup_mode, "%u");
> 
> While a bitwise 'and' operator surely works with booleans here you should still
> use a logical 'and' operator here.
ok

Best Regards,
J.
> 
> Sascha
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 4/5] boot: if we are in secure boot mode

[Sascha Hauer]

On Tue, Mar 14, 2017 at 09:07:35AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 08:55 Mon 13 Mar     , Sascha Hauer wrote:
> > On Thu, Mar 09, 2017 at 03:34:09PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > request confirmation before booting an unsigned image
> > > 
> > > with a default timeout
> > > 
> > > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > > ---
> > >  commands/go.c         |  7 +++++++
> > >  common/Kconfig        |  8 ++++++++
> > >  common/Makefile       |  1 +
> > >  common/bootm.c        |  7 +++++++
> > >  common/secure_boot.c  | 43 +++++++++++++++++++++++++++++++++++++++++++
> > >  include/bootm.h       |  1 +
> > >  include/secure_boot.h | 25 +++++++++++++++++++++++++
> > >  7 files changed, 92 insertions(+)
> > >  create mode 100644 common/secure_boot.c
> > >  create mode 100644 include/secure_boot.h
> > > 
> > > diff --git a/commands/go.c b/commands/go.c
> > > index fb319b320..61c9ce43f 100644
> > > --- a/commands/go.c
> > > +++ b/commands/go.c
> > > @@ -26,6 +26,7 @@
> > >  #include <fcntl.h>
> > >  #include <linux/ctype.h>
> > >  #include <errno.h>
> > > +#include <secure_boot.h>
> > >  
> > >  static int do_go(int argc, char *argv[])
> > >  {
> > > @@ -37,6 +38,12 @@ static int do_go(int argc, char *argv[])
> > >  	if (argc < 2)
> > >  		return COMMAND_ERROR_USAGE;
> > >  
> > > +	rcode = secure_boot_start_unsigned();
> > > +	if (rcode)
> > > +		return rcode ? 1 : 0;
> > > +
> > > +	rcode = 1;
> > > +
> > >  	if (!isdigit(*argv[1])) {
> > >  		fd = open(argv[1], O_RDONLY);
> > >  		if (fd < 0) {
> > > diff --git a/common/Kconfig b/common/Kconfig
> > > index f7ff04664..9e7d027a2 100644
> > > --- a/common/Kconfig
> > > +++ b/common/Kconfig
> > > @@ -21,6 +21,9 @@ config HAS_KALLSYMS
> > >  config HAS_MODULES
> > >  	bool
> > >  
> > > +config HAS_SECURE_BOOT
> > > +	bool
> > > +
> > >  config HAS_CACHE
> > >  	bool
> > >  	help
> > > @@ -184,6 +187,11 @@ config NVVAR
> > >  	  while global variables can be changed during runtime without changing the
> > >  	  default.
> > >  
> > > +config SECURE_BOOT_TIMEOUT
> > > +	prompt "Secure Boot unsigned boot timeout"
> > > +	int
> > > +	default 3
> > > +
> > >  menu "memory layout"
> > >  
> > >  source "pbl/Kconfig"
> > > diff --git a/common/Makefile b/common/Makefile
> > > index 5f58c81d2..e57cc2c15 100644
> > > --- a/common/Makefile
> > > +++ b/common/Makefile
> > > @@ -61,6 +61,7 @@ obj-$(CONFIG_UBIFORMAT)		+= ubiformat.o
> > >  obj-$(CONFIG_BAREBOX_UPDATE_IMX_NAND_FCB) += imx-bbu-nand-fcb.o
> > >  obj-$(CONFIG_CONSOLE_RATP)	+= ratp.o
> > >  obj-$(CONFIG_BOOT)		+= boot.o
> > > +obj-$(CONFIG_HAS_SECURE_BOOT)	+= secure_boot.o
> > >  
> > >  quiet_cmd_pwd_h = PWDH    $@
> > >  ifdef CONFIG_PASSWORD
> > > diff --git a/common/bootm.c b/common/bootm.c
> > > index 81625d915..0ebf65681 100644
> > > --- a/common/bootm.c
> > > +++ b/common/bootm.c
> > > @@ -23,6 +23,7 @@
> > >  #include <environment.h>
> > >  #include <linux/stat.h>
> > >  #include <magicvar.h>
> > > +#include <secure_boot.h>
> > >  
> > >  static LIST_HEAD(handler_list);
> > >  
> > > @@ -625,6 +626,12 @@ int bootm_boot(struct bootm_data *bootm_data)
> > >  		printf("Passing control to %s handler\n", handler->name);
> > >  	}
> > >  
> > > +	if (!handler->is_secure_supported && is_secure_mode()) {
> > > +		ret = secure_boot_start_unsigned();
> > > +		if (ret)
> > > +			goto err_out;
> > > +	}
> > 
> > We already have CONFIG_BOOTM_FORCE_SIGNED_IMAGES which admittedly is not
> > the most generic implementation, but in the end we shouldn't have two
> > different ways how secure boot is integrated in the bootm code.
> Did not seen it
> 
> I check it and this is only for BOOTM as we have go command too
> 
> that need to call image check too
> 
> and on efi this is not done at config time but runtime
> 
> and this is really FIT image oriented :(

You have the handler->is_secure_supported variable. This could be
implemented by the FIT image support aswell. Also we have the decision
if we want to boot unsigned images. Right now this is done in the
data->verify variable. We can force the decision in Kconfig using
CONFIG_BOOTM_FORCE_SIGNED_IMAGES or we can fill in data->verify before
booting.

> 
> Can we have one for EFI sperately first and then merge them as this will need
> a lot of testing and checking to ensure we break nothing

No. It's just too confusing to implement two different secure boot ways
in the same code and I give nothing to the promise that this will be
cleaned up later.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox