* [PATCH 1/4] efi/libstub: fix harmless command line parsing bug
2017-03-24 13:24 [PATCH 0/4] efi: libstub enhancements for cmdline parsing and kaslr Ard Biesheuvel
@ 2017-03-24 13:24 ` Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 2/4] efi/libstub: unify command line param parsing Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-24 13:24 UTC (permalink / raw)
To: linux-arm-kernel
When we parse the 'efi=' command line parameter in the stub, we
fail to take spaces into account. Currently, the only way this
could result in unexpected behavior is when the string 'nochunk'
appears as a separate command line argument after 'efi=xxx,yyy,zzz ',
so this is harmless in practice. But let's fix it nonetheless.
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
drivers/firmware/efi/libstub/efi-stub-helper.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 919822b7773d..3290fae0b38f 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -436,14 +436,14 @@ efi_status_t efi_parse_options(char *cmdline)
* Remember, because efi= is also used by the kernel we need to
* skip over arguments we don't understand.
*/
- while (*str) {
+ while (*str && *str != ' ') {
if (!strncmp(str, "nochunk", 7)) {
str += strlen("nochunk");
__chunk_size = -1UL;
}
/* Group words together, delimited by "," */
- while (*str && *str != ',')
+ while (*str && *str != ' ' && *str != ',')
str++;
if (*str == ',')
--
2.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 2/4] efi/libstub: unify command line param parsing
2017-03-24 13:24 [PATCH 0/4] efi: libstub enhancements for cmdline parsing and kaslr Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 1/4] efi/libstub: fix harmless command line parsing bug Ard Biesheuvel
@ 2017-03-24 13:24 ` Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 3/4] efi/libstub: arm/arm64: disable debug prints on 'quiet' cmdline arg Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region Ard Biesheuvel
3 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-24 13:24 UTC (permalink / raw)
To: linux-arm-kernel
Merge the parsing of the command line carried out in arm-stub.c with
the handling in efi_parse_options. Note that this also fixes the
missing handling of CONFIG_CMDLINE_FORCE=y, in which case the builtin
command line should supersede the one passed by the firmware.
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
drivers/firmware/efi/libstub/arm-stub.c | 14 ------------
drivers/firmware/efi/libstub/arm64-stub.c | 4 +---
drivers/firmware/efi/libstub/efi-stub-helper.c | 23 +++++++++++++-------
drivers/firmware/efi/libstub/efistub.h | 2 ++
include/linux/efi.h | 2 +-
5 files changed, 19 insertions(+), 26 deletions(-)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 02049ff25c6b..fcd34057dc1c 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -18,8 +18,6 @@
#include "efistub.h"
-bool __nokaslr;
-
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
void *__image, void **__fh)
{
@@ -153,18 +151,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
goto fail;
}
- /* check whether 'nokaslr' was passed on the command line */
- if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
- static const u8 default_cmdline[] = CONFIG_CMDLINE;
- const u8 *str, *cmdline = cmdline_ptr;
-
- if (IS_ENABLED(CONFIG_CMDLINE_FORCE))
- cmdline = default_cmdline;
- str = strstr(cmdline, "nokaslr");
- if (str == cmdline || (str > cmdline && *(str - 1) == ' '))
- __nokaslr = true;
- }
-
si = setup_graphics(sys_table);
status = handle_kernel_image(sys_table, image_addr, &image_size,
diff --git a/drivers/firmware/efi/libstub/arm64-stub.c b/drivers/firmware/efi/libstub/arm64-stub.c
index eae693eb3e91..b4c2589d7c91 100644
--- a/drivers/firmware/efi/libstub/arm64-stub.c
+++ b/drivers/firmware/efi/libstub/arm64-stub.c
@@ -16,8 +16,6 @@
#include "efistub.h"
-extern bool __nokaslr;
-
efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
{
u64 tg;
@@ -52,7 +50,7 @@ efi_status_t handle_kernel_image(efi_system_table_t *sys_table_arg,
u64 phys_seed = 0;
if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
- if (!__nokaslr) {
+ if (!nokaslr()) {
status = efi_get_random_bytes(sys_table_arg,
sizeof(phys_seed),
(u8 *)&phys_seed);
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 3290fae0b38f..1575b566cd4a 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -32,6 +32,13 @@
static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
+static int __section(.data) __nokaslr;
+
+int __pure nokaslr(void)
+{
+ return __nokaslr;
+}
+
#define EFI_MMAP_NR_SLACK_SLOTS 8
struct file_info {
@@ -409,17 +416,17 @@ static efi_status_t efi_file_close(void *handle)
* environments, first in the early boot environment of the EFI boot
* stub, and subsequently during the kernel boot.
*/
-efi_status_t efi_parse_options(char *cmdline)
+efi_status_t efi_parse_options(char const *cmdline)
{
+ static const char default_cmdline[] = CONFIG_CMDLINE;
char *str;
- /*
- * Currently, the only efi= option we look for is 'nochunk', which
- * is intended to work around known issues on certain x86 UEFI
- * versions. So ignore for now on other architectures.
- */
- if (!IS_ENABLED(CONFIG_X86))
- return EFI_SUCCESS;
+ if (IS_ENABLED(CONFIG_CMDLINE_FORCE))
+ cmdline = default_cmdline;
+
+ str = strstr(cmdline, "nokaslr");
+ if (str == cmdline || (str && str > cmdline && *(str - 1) == ' '))
+ __nokaslr = 1;
/*
* If no EFI parameters were specified on the cmdline we've got
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 71c4d0e3c4ed..a7a2a2c3f199 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -24,6 +24,8 @@
#define EFI_ALLOC_ALIGN EFI_PAGE_SIZE
#endif
+extern int __pure nokaslr(void);
+
void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg, void *__image,
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 94d34e0be24f..e485e87615d1 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1471,7 +1471,7 @@ efi_status_t handle_cmdline_files(efi_system_table_t *sys_table_arg,
unsigned long *load_addr,
unsigned long *load_size);
-efi_status_t efi_parse_options(char *cmdline);
+efi_status_t efi_parse_options(char const *cmdline);
efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
struct screen_info *si, efi_guid_t *proto,
--
2.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/4] efi/libstub: arm/arm64: disable debug prints on 'quiet' cmdline arg
2017-03-24 13:24 [PATCH 0/4] efi: libstub enhancements for cmdline parsing and kaslr Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 1/4] efi/libstub: fix harmless command line parsing bug Ard Biesheuvel
2017-03-24 13:24 ` [PATCH 2/4] efi/libstub: unify command line param parsing Ard Biesheuvel
@ 2017-03-24 13:24 ` Ard Biesheuvel
2017-03-24 14:15 ` Mark Rutland
2017-03-24 13:24 ` [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region Ard Biesheuvel
3 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-24 13:24 UTC (permalink / raw)
To: linux-arm-kernel
The EFI stub currently prints a number of diagnostic messages that do
not carry a lot of information. Since these prints are not controlled
by 'loglevel' or other command line parameters, and since they appear on
the EFI framebuffer as well (if enabled), it would be nice if we could
turn them off.
So let's add support for the 'quiet' command line parameter in the stub,
and disable the non-error prints if it is passed.
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
This was previously sent out as a separate patch. The difference is that this
version looks for 'quiet' rather than 'efi=debug', and preserves the noisy
console as the default.
drivers/firmware/efi/libstub/arm-stub.c | 12 ++++++------
drivers/firmware/efi/libstub/arm32-stub.c | 2 ++
drivers/firmware/efi/libstub/efi-stub-helper.c | 9 +++++++++
drivers/firmware/efi/libstub/efistub.h | 7 +++++++
drivers/firmware/efi/libstub/secureboot.c | 2 ++
include/linux/efi.h | 3 ---
6 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index fcd34057dc1c..6f522e3091af 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -116,8 +116,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
goto fail;
- pr_efi(sys_table, "Booting Linux Kernel...\n");
-
status = check_platform_features(sys_table);
if (status != EFI_SUCCESS)
goto fail;
@@ -151,6 +149,12 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
goto fail;
}
+ status = efi_parse_options(cmdline_ptr);
+ if (status != EFI_SUCCESS)
+ pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
+
+ pr_efi(sys_table, "Booting Linux Kernel...\n");
+
si = setup_graphics(sys_table);
status = handle_kernel_image(sys_table, image_addr, &image_size,
@@ -162,10 +166,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
goto fail_free_cmdline;
}
- status = efi_parse_options(cmdline_ptr);
- if (status != EFI_SUCCESS)
- pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
-
secure_boot = efi_get_secureboot(sys_table);
/*
diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
index 18a8b5eb55e7..becbda445913 100644
--- a/drivers/firmware/efi/libstub/arm32-stub.c
+++ b/drivers/firmware/efi/libstub/arm32-stub.c
@@ -9,6 +9,8 @@
#include <linux/efi.h>
#include <asm/efi.h>
+#include "efistub.h"
+
efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
{
int block;
diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
index 1575b566cd4a..93685f79f9e8 100644
--- a/drivers/firmware/efi/libstub/efi-stub-helper.c
+++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
@@ -33,11 +33,16 @@
static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
static int __section(.data) __nokaslr;
+static int __section(.data) __quiet;
int __pure nokaslr(void)
{
return __nokaslr;
}
+int __pure is_quiet(void)
+{
+ return __quiet;
+}
#define EFI_MMAP_NR_SLACK_SLOTS 8
@@ -428,6 +433,10 @@ efi_status_t efi_parse_options(char const *cmdline)
if (str == cmdline || (str && str > cmdline && *(str - 1) == ' '))
__nokaslr = 1;
+ str = strstr(cmdline, "quiet");
+ if (str == cmdline || (str && str > cmdline && *(str - 1) == ' '))
+ __quiet = 1;
+
/*
* If no EFI parameters were specified on the cmdline we've got
* nothing to do.
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index a7a2a2c3f199..83f268c05007 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -25,6 +25,13 @@
#endif
extern int __pure nokaslr(void);
+extern int __pure is_quiet(void);
+
+#define pr_efi(sys_table, msg) do { \
+ if (!is_quiet()) efi_printk(sys_table, "EFI stub: "msg); \
+} while (0)
+
+#define pr_efi_err(sys_table, msg) efi_printk(sys_table, "EFI stub: ERROR: "msg)
void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 5da36e56b36a..8c34d50a4d80 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -12,6 +12,8 @@
#include <linux/efi.h>
#include <asm/efi.h>
+#include "efistub.h"
+
/* BIOS variables */
static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
static const efi_char16_t const efi_SecureBoot_name[] = {
diff --git a/include/linux/efi.h b/include/linux/efi.h
index e485e87615d1..ec36f42a2add 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1435,9 +1435,6 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz)
/* prototypes shared between arch specific and generic stub code */
-#define pr_efi(sys_table, msg) efi_printk(sys_table, "EFI stub: "msg)
-#define pr_efi_err(sys_table, msg) efi_printk(sys_table, "EFI stub: ERROR: "msg)
-
void efi_printk(efi_system_table_t *sys_table_arg, char *str);
void efi_free(efi_system_table_t *sys_table_arg, unsigned long size,
--
2.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 3/4] efi/libstub: arm/arm64: disable debug prints on 'quiet' cmdline arg
2017-03-24 13:24 ` [PATCH 3/4] efi/libstub: arm/arm64: disable debug prints on 'quiet' cmdline arg Ard Biesheuvel
@ 2017-03-24 14:15 ` Mark Rutland
0 siblings, 0 replies; 11+ messages in thread
From: Mark Rutland @ 2017-03-24 14:15 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Mar 24, 2017 at 01:24:09PM +0000, Ard Biesheuvel wrote:
> The EFI stub currently prints a number of diagnostic messages that do
> not carry a lot of information. Since these prints are not controlled
> by 'loglevel' or other command line parameters, and since they appear on
> the EFI framebuffer as well (if enabled), it would be nice if we could
> turn them off.
>
> So let's add support for the 'quiet' command line parameter in the stub,
> and disable the non-error prints if it is passed.
>
> Cc: Matt Fleming <matt@codeblueprint.co.uk>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> This was previously sent out as a separate patch. The difference is that this
> version looks for 'quiet' rather than 'efi=debug', and preserves the noisy
> console as the default.
This addresses my prior concern, so FWIW:
Acked-by: Mark Rutland <mark.rutland@arm.com>
Mark.
> drivers/firmware/efi/libstub/arm-stub.c | 12 ++++++------
> drivers/firmware/efi/libstub/arm32-stub.c | 2 ++
> drivers/firmware/efi/libstub/efi-stub-helper.c | 9 +++++++++
> drivers/firmware/efi/libstub/efistub.h | 7 +++++++
> drivers/firmware/efi/libstub/secureboot.c | 2 ++
> include/linux/efi.h | 3 ---
> 6 files changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index fcd34057dc1c..6f522e3091af 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -116,8 +116,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> if (sys_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
> goto fail;
>
> - pr_efi(sys_table, "Booting Linux Kernel...\n");
> -
> status = check_platform_features(sys_table);
> if (status != EFI_SUCCESS)
> goto fail;
> @@ -151,6 +149,12 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> goto fail;
> }
>
> + status = efi_parse_options(cmdline_ptr);
> + if (status != EFI_SUCCESS)
> + pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
> +
> + pr_efi(sys_table, "Booting Linux Kernel...\n");
> +
> si = setup_graphics(sys_table);
>
> status = handle_kernel_image(sys_table, image_addr, &image_size,
> @@ -162,10 +166,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> goto fail_free_cmdline;
> }
>
> - status = efi_parse_options(cmdline_ptr);
> - if (status != EFI_SUCCESS)
> - pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
> -
> secure_boot = efi_get_secureboot(sys_table);
>
> /*
> diff --git a/drivers/firmware/efi/libstub/arm32-stub.c b/drivers/firmware/efi/libstub/arm32-stub.c
> index 18a8b5eb55e7..becbda445913 100644
> --- a/drivers/firmware/efi/libstub/arm32-stub.c
> +++ b/drivers/firmware/efi/libstub/arm32-stub.c
> @@ -9,6 +9,8 @@
> #include <linux/efi.h>
> #include <asm/efi.h>
>
> +#include "efistub.h"
> +
> efi_status_t check_platform_features(efi_system_table_t *sys_table_arg)
> {
> int block;
> diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> index 1575b566cd4a..93685f79f9e8 100644
> --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> @@ -33,11 +33,16 @@
> static unsigned long __chunk_size = EFI_READ_CHUNK_SIZE;
>
> static int __section(.data) __nokaslr;
> +static int __section(.data) __quiet;
>
> int __pure nokaslr(void)
> {
> return __nokaslr;
> }
> +int __pure is_quiet(void)
> +{
> + return __quiet;
> +}
>
> #define EFI_MMAP_NR_SLACK_SLOTS 8
>
> @@ -428,6 +433,10 @@ efi_status_t efi_parse_options(char const *cmdline)
> if (str == cmdline || (str && str > cmdline && *(str - 1) == ' '))
> __nokaslr = 1;
>
> + str = strstr(cmdline, "quiet");
> + if (str == cmdline || (str && str > cmdline && *(str - 1) == ' '))
> + __quiet = 1;
> +
> /*
> * If no EFI parameters were specified on the cmdline we've got
> * nothing to do.
> diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
> index a7a2a2c3f199..83f268c05007 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -25,6 +25,13 @@
> #endif
>
> extern int __pure nokaslr(void);
> +extern int __pure is_quiet(void);
> +
> +#define pr_efi(sys_table, msg) do { \
> + if (!is_quiet()) efi_printk(sys_table, "EFI stub: "msg); \
> +} while (0)
> +
> +#define pr_efi_err(sys_table, msg) efi_printk(sys_table, "EFI stub: ERROR: "msg)
>
> void efi_char16_printk(efi_system_table_t *, efi_char16_t *);
>
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index 5da36e56b36a..8c34d50a4d80 100644
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -12,6 +12,8 @@
> #include <linux/efi.h>
> #include <asm/efi.h>
>
> +#include "efistub.h"
> +
> /* BIOS variables */
> static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
> static const efi_char16_t const efi_SecureBoot_name[] = {
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index e485e87615d1..ec36f42a2add 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1435,9 +1435,6 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz)
>
> /* prototypes shared between arch specific and generic stub code */
>
> -#define pr_efi(sys_table, msg) efi_printk(sys_table, "EFI stub: "msg)
> -#define pr_efi_err(sys_table, msg) efi_printk(sys_table, "EFI stub: ERROR: "msg)
> -
> void efi_printk(efi_system_table_t *sys_table_arg, char *str);
>
> void efi_free(efi_system_table_t *sys_table_arg, unsigned long size,
> --
> 2.9.3
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-03-24 13:24 [PATCH 0/4] efi: libstub enhancements for cmdline parsing and kaslr Ard Biesheuvel
` (2 preceding siblings ...)
2017-03-24 13:24 ` [PATCH 3/4] efi/libstub: arm/arm64: disable debug prints on 'quiet' cmdline arg Ard Biesheuvel
@ 2017-03-24 13:24 ` Ard Biesheuvel
2017-04-07 15:47 ` James Morse
3 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2017-03-24 13:24 UTC (permalink / raw)
To: linux-arm-kernel
Update the allocation logic for the virtual mapping of the UEFI runtime
services to start from a randomized base address if KASLR is in effect,
and if the UEFI firmware exposes an implementation of EFI_RNG_PROTOCOL.
This makes it more difficult to predict the location of exploitable
data structures in the runtime UEFI firmware, which increases robustness
against attacks. Note that these regions are only mapped during the
time a runtime service call is in progress, and only on a single CPU
at a time, bit give the lack of a downside, let's enable it nonetheless.
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Matt Fleming <matt@codeblueprint.co.uk>
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
drivers/firmware/efi/libstub/arm-stub.c | 48 ++++++++++++++------
1 file changed, 35 insertions(+), 13 deletions(-)
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index 6f522e3091af..eb5225884098 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -18,6 +18,22 @@
#include "efistub.h"
+/*
+ * This is the base address at which to start allocating virtual memory ranges
+ * for UEFI Runtime Services. This is in the low TTBR0 range so that we can use
+ * any allocation we choose, and eliminate the risk of a conflict after kexec.
+ * The value chosen is the largest non-zero power of 2 suitable for this purpose
+ * both on 32-bit and 64-bit ARM CPUs, to maximize the likelihood that it can
+ * be mapped efficiently.
+ * Since 32-bit ARM could potentially execute with a 1G/3G user/kernel split,
+ * map everything below 1 GB. (512 MB is a reasonable upper bound for the
+ * entire footprint of the UEFI runtime services memory regions)
+ */
+#define EFI_RT_VIRTUAL_BASE SZ_512M
+#define EFI_RT_VIRTUAL_SIZE SZ_512M
+
+static u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
+
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
void *__image, void **__fh)
{
@@ -209,6 +225,25 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
efi_random_get_seed(sys_table);
+ if (IS_ENABLED(CONFIG_RANDOMIZE_BASE) && !nokaslr()) {
+ /*
+ * Randomize the base of the UEFI runtime services region.
+ * Preserve the 2 MB alignment of the region by taking a
+ * shift of 21 bit positions into account when scaling
+ * the headroom value using a 32-bit random value.
+ */
+ u64 headroom = TASK_SIZE - EFI_RT_VIRTUAL_BASE -
+ EFI_RT_VIRTUAL_SIZE;
+ u32 rnd;
+
+ status = efi_get_random_bytes(sys_table, sizeof(rnd),
+ (u8 *)&rnd);
+ if (status == EFI_SUCCESS) {
+ efi_virt_base = EFI_RT_VIRTUAL_BASE +
+ (((headroom >> 21) * rnd) >> (32 - 21));
+ }
+ }
+
new_fdt_addr = fdt_addr;
status = allocate_new_fdt_and_exit_boot(sys_table, handle,
&new_fdt_addr, efi_get_max_fdt_addr(dram_base),
@@ -238,18 +273,6 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
return EFI_ERROR;
}
-/*
- * This is the base address at which to start allocating virtual memory ranges
- * for UEFI Runtime Services. This is in the low TTBR0 range so that we can use
- * any allocation we choose, and eliminate the risk of a conflict after kexec.
- * The value chosen is the largest non-zero power of 2 suitable for this purpose
- * both on 32-bit and 64-bit ARM CPUs, to maximize the likelihood that it can
- * be mapped efficiently.
- * Since 32-bit ARM could potentially execute with a 1G/3G user/kernel split,
- * map everything below 1 GB.
- */
-#define EFI_RT_VIRTUAL_BASE SZ_512M
-
static int cmp_mem_desc(const void *l, const void *r)
{
const efi_memory_desc_t *left = l, *right = r;
@@ -299,7 +322,6 @@ void efi_get_virtmap(efi_memory_desc_t *memory_map, unsigned long map_size,
unsigned long desc_size, efi_memory_desc_t *runtime_map,
int *count)
{
- u64 efi_virt_base = EFI_RT_VIRTUAL_BASE;
efi_memory_desc_t *in, *prev = NULL, *out = runtime_map;
int l;
--
2.9.3
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-03-24 13:24 ` [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region Ard Biesheuvel
@ 2017-04-07 15:47 ` James Morse
2017-04-07 15:51 ` Ard Biesheuvel
0 siblings, 1 reply; 11+ messages in thread
From: James Morse @ 2017-04-07 15:47 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ard,
On 24/03/17 13:24, Ard Biesheuvel wrote:
> Update the allocation logic for the virtual mapping of the UEFI runtime
> services to start from a randomized base address if KASLR is in effect,
> and if the UEFI firmware exposes an implementation of EFI_RNG_PROTOCOL.
>
> This makes it more difficult to predict the location of exploitable
> data structures in the runtime UEFI firmware, which increases robustness
> against attacks. Note that these regions are only mapped during the
> time a runtime service call is in progress, and only on a single CPU
> at a time, bit give the lack of a downside, let's enable it nonetheless.
With next-20170407 on Seattle Overdrive, I get an SError[0] on boot:
The three patches I have on top of 4.11.0-rc5-next-20170407 are:
* Revert "efi/libstub/arm*: Set default address and size cells values for an
empty dtb"
* Revert "ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
At which point the machine start booting to a prompt again, (its noisier than
usual but looks like double-printing).
If I then cherry-pick:
* ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
It again fails, producing the trace below. This is all with next-20170407's
defconfig, 4K/48. UEFI identifies itself as:
> UEFI v2.40 (American Megatrends, 0x0005000B)
Thanks,
James
[0]
Shell> efi\morse\Image console=ttyAMA0,115200 root=PARTUUID=b2edf709-3b28-4cb3-8
809-203f262e2bcc rw earlycon=pl011,0xe1010000 crashkernel=1G stacktrace ignore_l
oglevel=1 acpi=on efi=debug resume=/dev/sda3
EFI stub: Booting Linux Kernel...
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[ 0.000000] Booting Linux on physical CPU 0x0
[ 0.000000] Linux version 4.11.0-rc5-next-20170407-00003-gbe65d54f8671 (morse
@melchizedek) (gcc version 4.9.3 20141031 (prerelease) (Linaro GCC 2014.11) ) #7
401 SMP PREEMPT Fri Apr 7 16:19:28 BST 2017
[ 0.000000] Boot CPU: AArch64 Processor [411fd072]
[ 0.000000] earlycon: pl11 at MMIO 0x00000000e1010000 (options '')
[ 0.000000] bootconsole [pl11] enabled
[ 0.000000] debug: ignoring loglevel setting.
[ 0.000000] Bad mode in Error handler detected on CPU0, code 0xbf000000 -- SError
[ 0.000000] Internal error: Oops - bad mode: 0 [#1] PREEMPT SMP
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.11.0-rc5-next-20170407-
00003-gbe65d54f8671 #7401
[ 0.000000] Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive)
(DT)
[ 0.000000] task: ffff000008e02b80 task.stack: ffff000008df0000
[ 0.000000] PC is at setup_arch+0xf0/0x504
[ 0.000000] LR is at setup_arch+0xec/0x504
[ 0.000000] pc : [<ffff000008ce2844>] lr : [<ffff000008ce2840>] pstate: 000000c5
[ 0.000000] Call trace:
[ 0.000000] [<ffff000008ce2844>] setup_arch+0xf0/0x504
[ 0.000000] [<ffff000008ce0838>] start_kernel+0x70/0x398
[ 0.000000] [<ffff000008ce01e0>] __primary_switched+0x64/0x74
[ 0.000000] Code: 9111c000 940034d9 97fff7cd d50344ff (90fffce3)
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
[ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-04-07 15:47 ` James Morse
@ 2017-04-07 15:51 ` Ard Biesheuvel
2017-04-07 16:11 ` James Morse
2017-04-10 9:41 ` Mark Rutland
0 siblings, 2 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-04-07 15:51 UTC (permalink / raw)
To: linux-arm-kernel
On 7 April 2017 at 16:47, James Morse <james.morse@arm.com> wrote:
> Hi Ard,
>
> On 24/03/17 13:24, Ard Biesheuvel wrote:
>> Update the allocation logic for the virtual mapping of the UEFI runtime
>> services to start from a randomized base address if KASLR is in effect,
>> and if the UEFI firmware exposes an implementation of EFI_RNG_PROTOCOL.
>>
>> This makes it more difficult to predict the location of exploitable
>> data structures in the runtime UEFI firmware, which increases robustness
>> against attacks. Note that these regions are only mapped during the
>> time a runtime service call is in progress, and only on a single CPU
>> at a time, bit give the lack of a downside, let's enable it nonetheless.
>
> With next-20170407 on Seattle Overdrive, I get an SError[0] on boot:
>
> The three patches I have on top of 4.11.0-rc5-next-20170407 are:
> * Revert "efi/libstub/arm*: Set default address and size cells values for an
> empty dtb"
> * Revert "ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
>
> At which point the machine start booting to a prompt again, (its noisier than
> usual but looks like double-printing).
>
> If I then cherry-pick:
> * ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
>
That is quite interesting, to be honest, because that patch should
effectively be a NOP on systems that do not implement
EFI_RNG_PROTOCOL.
Could you run this from the UEFI shell please?
http://people.linaro.org/~ard.biesheuvel/RngTest.efi
I would expect it to report that it has no EFI_RNG_PROTOCOL
implementation. Could you also check whether the working kernel still
works /after/ having executed that utility?
> It again fails, producing the trace below. This is all with next-20170407's
> defconfig, 4K/48. UEFI identifies itself as:
>> UEFI v2.40 (American Megatrends, 0x0005000B)
>
>
> Thanks,
>
> James
>
>
> [0]
> Shell> efi\morse\Image console=ttyAMA0,115200 root=PARTUUID=b2edf709-3b28-4cb3-8
> 809-203f262e2bcc rw earlycon=pl011,0xe1010000 crashkernel=1G stacktrace ignore_l
> oglevel=1 acpi=on efi=debug resume=/dev/sda3
> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
> EFI stub: Exiting boot services and installing virtual address map...
> [ 0.000000] Booting Linux on physical CPU 0x0
> [ 0.000000] Linux version 4.11.0-rc5-next-20170407-00003-gbe65d54f8671 (morse
> @melchizedek) (gcc version 4.9.3 20141031 (prerelease) (Linaro GCC 2014.11) ) #7
> 401 SMP PREEMPT Fri Apr 7 16:19:28 BST 2017
> [ 0.000000] Boot CPU: AArch64 Processor [411fd072]
> [ 0.000000] earlycon: pl11 at MMIO 0x00000000e1010000 (options '')
> [ 0.000000] bootconsole [pl11] enabled
> [ 0.000000] debug: ignoring loglevel setting.
> [ 0.000000] Bad mode in Error handler detected on CPU0, code 0xbf000000 -- SError
> [ 0.000000] Internal error: Oops - bad mode: 0 [#1] PREEMPT SMP
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 4.11.0-rc5-next-20170407-
> 00003-gbe65d54f8671 #7401
> [ 0.000000] Hardware name: AMD Seattle (Rev.B0) Development Board (Overdrive)
> (DT)
> [ 0.000000] task: ffff000008e02b80 task.stack: ffff000008df0000
> [ 0.000000] PC is at setup_arch+0xf0/0x504
> [ 0.000000] LR is at setup_arch+0xec/0x504
> [ 0.000000] pc : [<ffff000008ce2844>] lr : [<ffff000008ce2840>] pstate: 000000c5
>
> [ 0.000000] Call trace:
> [ 0.000000] [<ffff000008ce2844>] setup_arch+0xf0/0x504
> [ 0.000000] [<ffff000008ce0838>] start_kernel+0x70/0x398
> [ 0.000000] [<ffff000008ce01e0>] __primary_switched+0x64/0x74
> [ 0.000000] Code: 9111c000 940034d9 97fff7cd d50344ff (90fffce3)
> [ 0.000000] ---[ end trace 0000000000000000 ]---
> [ 0.000000] Kernel panic - not syncing: Attempted to kill the idle task!
> [ 0.000000] ---[ end Kernel panic - not syncing: Attempted to kill the idle task!
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-04-07 15:51 ` Ard Biesheuvel
@ 2017-04-07 16:11 ` James Morse
2017-04-10 9:41 ` Mark Rutland
1 sibling, 0 replies; 11+ messages in thread
From: James Morse @ 2017-04-07 16:11 UTC (permalink / raw)
To: linux-arm-kernel
Hi Ard,
On 07/04/17 16:51, Ard Biesheuvel wrote:
> That is quite interesting, to be honest, because that patch should
> effectively be a NOP on systems that do not implement
> EFI_RNG_PROTOCOL.
>
> Could you run this from the UEFI shell please?
>
> http://people.linaro.org/~ard.biesheuvel/RngTest.efi
As you predicted:
Shell> RngTest.efi
UEFI RNG Protocol Testing :
----------------------------
-- Locate UEFI RNG Protocol : [Fail - Status = Not Found]
Error reported: Not Found
> I would expect it to report that it has no EFI_RNG_PROTOCOL
> implementation. Could you also check whether the working kernel still
> works /after/ having executed that utility?
The broken kernel remains broken after running that test. reboot. The working
kernel continues to work after running that test.
(On Monday) I will try with just these efi changes on v4.11-rc. to try and
eliminate everything else in linux-next.
This is one of those firmware versions that prints lots of
> efi: [Firmware Bug]: IRQ flags corrupted (0x00000140=>0x00000100) by EFI
get_variable
Thanks,
James
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-04-07 15:51 ` Ard Biesheuvel
2017-04-07 16:11 ` James Morse
@ 2017-04-10 9:41 ` Mark Rutland
2017-04-10 9:44 ` Ard Biesheuvel
1 sibling, 1 reply; 11+ messages in thread
From: Mark Rutland @ 2017-04-10 9:41 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Apr 07, 2017 at 04:51:16PM +0100, Ard Biesheuvel wrote:
> On 7 April 2017 at 16:47, James Morse <james.morse@arm.com> wrote:
> > On 24/03/17 13:24, Ard Biesheuvel wrote:
> >> Update the allocation logic for the virtual mapping of the UEFI runtime
> >> services to start from a randomized base address if KASLR is in effect,
> >> and if the UEFI firmware exposes an implementation of EFI_RNG_PROTOCOL.
> >>
> >> This makes it more difficult to predict the location of exploitable
> >> data structures in the runtime UEFI firmware, which increases robustness
> >> against attacks. Note that these regions are only mapped during the
> >> time a runtime service call is in progress, and only on a single CPU
> >> at a time, bit give the lack of a downside, let's enable it nonetheless.
> >
> > With next-20170407 on Seattle Overdrive, I get an SError[0] on boot:
> > * Revert "ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
> >
> > At which point the machine start booting to a prompt again, (its noisier than
> > usual but looks like double-printing).
> That is quite interesting, to be honest, because that patch should
> effectively be a NOP on systems that do not implement
> EFI_RNG_PROTOCOL.
FWIW, I'm also seeing a crash as a result of this patch, on a Juno R1
with an EFI_RNG_PROTOCOL implementation, with next-20170410 defconfig.
I'm using the Linaro 15.08 toolchain.
----
EFI stub: Booting Linux Kernel...
EFI stub: Using DTB from configuration table
WARNING: this system is using an unsafe pseudo-random implementation of
RngLib!
Synchronous Exception at 0x00000000F80FC7E8
----
If I replace the !nokaslr() in efi_entry() with false, my kernel boots.
If I replace !nokaslr() with true, it fails to boot, even with the call
to efi_get_random_bytes() replaced with status = EFI_ERROR.
The problem appears to be the use of TASK_SIZE, since on arm64 that's:
#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
TASK_SIZE_32 : TASK_SIZE_64)
... which would mean that the stub would be trying to dereference whatever it
found in sp_el0. Looking at objdump:
0000000000000160 <efi_entry>:
...
4b4: 94000000 bl 0 <nokaslr>
4b8: 35000280 cbnz w0, 508 <efi_entry+0x3a8>
4bc: d5384100 mrs x0, sp_el0
4c0: f9400004 ldr x4, [x0]
...
So I think we should revert this for now.
Thanks,
Mark.
^ permalink raw reply [flat|nested] 11+ messages in thread* [PATCH 4/4] ef/libstub: arm/arm64: randomize the base of the UEFI rt services region
2017-04-10 9:41 ` Mark Rutland
@ 2017-04-10 9:44 ` Ard Biesheuvel
0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2017-04-10 9:44 UTC (permalink / raw)
To: linux-arm-kernel
On 10 April 2017 at 10:41, Mark Rutland <mark.rutland@arm.com> wrote:
> On Fri, Apr 07, 2017 at 04:51:16PM +0100, Ard Biesheuvel wrote:
>> On 7 April 2017 at 16:47, James Morse <james.morse@arm.com> wrote:
>> > On 24/03/17 13:24, Ard Biesheuvel wrote:
>> >> Update the allocation logic for the virtual mapping of the UEFI runtime
>> >> services to start from a randomized base address if KASLR is in effect,
>> >> and if the UEFI firmware exposes an implementation of EFI_RNG_PROTOCOL.
>> >>
>> >> This makes it more difficult to predict the location of exploitable
>> >> data structures in the runtime UEFI firmware, which increases robustness
>> >> against attacks. Note that these regions are only mapped during the
>> >> time a runtime service call is in progress, and only on a single CPU
>> >> at a time, bit give the lack of a downside, let's enable it nonetheless.
>> >
>> > With next-20170407 on Seattle Overdrive, I get an SError[0] on boot:
>
>> > * Revert "ef/libstub/arm/arm64: Randomize the base of the UEFI rt services region"
>> >
>> > At which point the machine start booting to a prompt again, (its noisier than
>> > usual but looks like double-printing).
>
>> That is quite interesting, to be honest, because that patch should
>> effectively be a NOP on systems that do not implement
>> EFI_RNG_PROTOCOL.
>
> FWIW, I'm also seeing a crash as a result of this patch, on a Juno R1
> with an EFI_RNG_PROTOCOL implementation, with next-20170410 defconfig.
> I'm using the Linaro 15.08 toolchain.
>
> ----
> EFI stub: Booting Linux Kernel...
> EFI stub: Using DTB from configuration table
>
>
> WARNING: this system is using an unsafe pseudo-random implementation of
> RngLib!
>
>
>
> Synchronous Exception at 0x00000000F80FC7E8
> ----
>
> If I replace the !nokaslr() in efi_entry() with false, my kernel boots.
>
> If I replace !nokaslr() with true, it fails to boot, even with the call
> to efi_get_random_bytes() replaced with status = EFI_ERROR.
>
> The problem appears to be the use of TASK_SIZE, since on arm64 that's:
>
> #define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
> TASK_SIZE_32 : TASK_SIZE_64)
>
> ... which would mean that the stub would be trying to dereference whatever it
> found in sp_el0. Looking at objdump:
>
> 0000000000000160 <efi_entry>:
> ...
> 4b4: 94000000 bl 0 <nokaslr>
> 4b8: 35000280 cbnz w0, 508 <efi_entry+0x3a8>
> 4bc: d5384100 mrs x0, sp_el0
> 4c0: f9400004 ldr x4, [x0]
> ...
>
> So I think we should revert this for now.
>
Thanks for debugging this! I will get a fixed version out asap
^ permalink raw reply [flat|nested] 11+ messages in thread