[PATCH 0/3] sandbox: exception handling

[PATCH 0/3] sandbox: exception handling

[Heinrich Schuchardt]

Currently if an exception SIGILL, SIGBUS, SIGSEGV occurs the sandbox
stops execution. This does not match the behavior on other architectures.

Instead print the current program counter and if any the involved UEFI
binaries. Then reset the system.

When testing UEFI binaries like the Self Certification Test exceptions may
occur. Without information about the UEFI binary where the exception
was caused it is difficult to analyze the cause.

The exception command is implemented for the sandbox. This command allows
to trigger SIGILL or SIGSEGV.

The UEFI exception unit test is implemented for the sandbox.

Heinrich Schuchardt (3):
  sandbox: add handler for exceptions
  cmd: sandbox: implement exception command
  efi_selftest: implement exception test for sandbox

 arch/sandbox/cpu/os.c                         | 31 ++++++++++++++
 arch/sandbox/cpu/start.c                      |  4 ++
 arch/sandbox/lib/interrupts.c                 | 30 ++++++++++++++
 cmd/Kconfig                                   |  2 +-
 cmd/Makefile                                  |  1 +
 cmd/sandbox/Makefile                          |  3 ++
 cmd/sandbox/exception.c                       | 41 +++++++++++++++++++
 include/os.h                                  | 17 ++++++++
 .../efi_selftest_miniapp_exception.c          |  2 +
 9 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 cmd/sandbox/Makefile
 create mode 100644 cmd/sandbox/exception.c

--
2.28.0

[PATCH 1/3] sandbox: add handler for exceptions

[Heinrich Schuchardt]

Add a handler for SIGILL, SIGBUS, SIGSEGV.

When an exception occurs print the program counter and the loaded
UEFI binaries and reset the system.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 arch/sandbox/cpu/os.c         | 31 +++++++++++++++++++++++++++++++
 arch/sandbox/cpu/start.c      |  4 ++++
 arch/sandbox/lib/interrupts.c | 30 ++++++++++++++++++++++++++++++
 include/os.h                  | 17 +++++++++++++++++
 4 files changed, 82 insertions(+)

diff --git a/arch/sandbox/cpu/os.c b/arch/sandbox/cpu/os.c
index 0d8efd83f6..d1c2c9fddd 100644
--- a/arch/sandbox/cpu/os.c
+++ b/arch/sandbox/cpu/os.c
@@ -3,6 +3,8 @@
  * Copyright (c) 2011 The Chromium OS Authors.
  */

+#define _GNU_SOURCE
+
 #include <dirent.h>
 #include <errno.h>
 #include <fcntl.h>
@@ -15,6 +17,7 @@
 #include <string.h>
 #include <termios.h>
 #include <time.h>
+#include <ucontext.h>
 #include <unistd.h>
 #include <sys/mman.h>
 #include <sys/stat.h>
@@ -191,6 +194,34 @@ static void os_sigint_handler(int sig)
 	raise(SIGINT);
 }

+static void os_signal_handler(int sig, siginfo_t *info, void *con)
+{
+	ucontext_t *context = con;
+	unsigned long pc = 0;
+
+#ifdef __x86_64__
+	pc = context->uc_mcontext.gregs[REG_RIP];
+#elif __aarch64__
+	pc = context->uc_mcontext.pc;
+#endif
+
+	os_signal_action(sig, pc);
+}
+
+int os_setup_signal_handlers(void)
+{
+	struct sigaction act;
+
+	act.sa_sigaction = os_signal_handler;
+	sigemptyset(&act.sa_mask);
+	act.sa_flags = SA_SIGINFO;
+	if (sigaction(SIGILL, &act, NULL) ||
+	    sigaction(SIGBUS, &act, NULL) ||
+	    sigaction(SIGSEGV, &act, NULL))
+		return -1;
+	return 0;
+}
+
 /* Put tty into raw mode so <tab> and <ctrl+c> work */
 void os_tty_raw(int fd, bool allow_sigs)
 {
diff --git a/arch/sandbox/cpu/start.c b/arch/sandbox/cpu/start.c
index a03e5aa0b3..f6c98545e0 100644
--- a/arch/sandbox/cpu/start.c
+++ b/arch/sandbox/cpu/start.c
@@ -451,6 +451,10 @@ int main(int argc, char *argv[])
 	if (ret)
 		goto err;

+	ret = os_setup_signal_handlers();
+	if (ret)
+		goto err;
+
 #if CONFIG_VAL(SYS_MALLOC_F_LEN)
 	gd->malloc_base = CONFIG_MALLOC_F_ADDR;
 #endif
diff --git a/arch/sandbox/lib/interrupts.c b/arch/sandbox/lib/interrupts.c
index 21f761ac3b..3c8f3df715 100644
--- a/arch/sandbox/lib/interrupts.c
+++ b/arch/sandbox/lib/interrupts.c
@@ -6,7 +6,13 @@
  */

 #include <common.h>
+#include <efi_loader.h>
 #include <irq_func.h>
+#include <os.h>
+#include <asm-generic/signal.h>
+#include <asm/u-boot-sandbox.h>
+
+DECLARE_GLOBAL_DATA_PTR;

 int interrupt_init(void)
 {
@@ -21,3 +27,27 @@ int disable_interrupts(void)
 {
 	return 0;
 }
+
+void os_signal_action(int sig, unsigned long pc)
+{
+	efi_restore_gd();
+
+	switch (sig) {
+	case SIGILL:
+		printf("Illegal instruction\n");
+		break;
+	case SIGBUS:
+		printf("Bus error\n");
+		break;
+	case SIGSEGV:
+		printf("Segmentation violation\n");
+		break;
+	default:
+		break;
+	}
+	printf("pc = 0x%lx, ", pc);
+	printf("pc_reloc = 0x%lx\n", pc - gd->reloc_off);
+	efi_print_image_infos((void *)pc);
+	printf("Resetting ...\n\n");
+	sandbox_reset();
+}
diff --git a/include/os.h b/include/os.h
index 1fe44f3510..0913b47b3a 100644
--- a/include/os.h
+++ b/include/os.h
@@ -407,4 +407,21 @@ void *os_find_text_base(void);
  */
 void os_relaunch(char *argv[]);

+/**
+ * os_setup_signal_handlers() - setup signal handlers
+ *
+ * Install signal handlers for SIGBUS and SIGSEGV.
+ *
+ * Return:	0 for success
+ */
+int os_setup_signal_handlers(void);
+
+/**
+ * os_signal_action() - handle a signal
+ *
+ * @sig:	signal
+ * @pc:		program counter
+ */
+void os_signal_action(int sig, unsigned long pc);
+
 #endif
--
2.28.0

[PATCH 2/3] cmd: sandbox: implement exception command

[Heinrich Schuchardt]

Implement the commands

* exception undefined - execute an illegal instruction
* exception sigsegv - cause a segment violation

Here is a possible output:

    => exception undefined
    Illegal instruction
    pc = 0x55eb8d0a7575, pc_reloc = 0x57575
    Resetting ...

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/Kconfig             |  2 +-
 cmd/Makefile            |  1 +
 cmd/sandbox/Makefile    |  3 +++
 cmd/sandbox/exception.c | 41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 46 insertions(+), 1 deletion(-)
 create mode 100644 cmd/sandbox/Makefile
 create mode 100644 cmd/sandbox/exception.c

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 1595de999b..f9b72449c5 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -1682,7 +1682,7 @@ config CMD_EFIDEBUG

 config CMD_EXCEPTION
 	bool "exception - raise exception"
-	depends on ARM || RISCV || X86
+	depends on ARM || RISCV || SANDBOX || X86
 	help
 	  Enable the 'exception' command which allows to raise an exception.

diff --git a/cmd/Makefile b/cmd/Makefile
index dd86675bf2..5b3400a840 100644
--- a/cmd/Makefile
+++ b/cmd/Makefile
@@ -193,6 +193,7 @@ obj-$(CONFIG_CMD_AVB) += avb.o

 obj-$(CONFIG_ARM) += arm/
 obj-$(CONFIG_RISCV) += riscv/
+obj-$(CONFIG_SANDBOX) += sandbox/
 obj-$(CONFIG_X86) += x86/

 obj-$(CONFIG_ARCH_MVEBU) += mvebu/
diff --git a/cmd/sandbox/Makefile b/cmd/sandbox/Makefile
new file mode 100644
index 0000000000..24df023ece
--- /dev/null
+++ b/cmd/sandbox/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+obj-$(CONFIG_CMD_EXCEPTION) += exception.o
diff --git a/cmd/sandbox/exception.c b/cmd/sandbox/exception.c
new file mode 100644
index 0000000000..1aa1d673ae
--- /dev/null
+++ b/cmd/sandbox/exception.c
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * The 'exception' command can be used for testing exception handling.
+ *
+ * Copyright (c) 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#include <common.h>
+#include <command.h>
+
+static int do_sigsegv(struct cmd_tbl *cmdtp, int flag, int argc,
+		      char *const argv[])
+{
+	u8 *ptr = NULL;
+
+	*ptr = 0;
+	return CMD_RET_FAILURE;
+}
+
+static int do_undefined(struct cmd_tbl *cmdtp, int flag, int argc,
+			char *const argv[])
+{
+	asm volatile (".word 0xffff\n");
+	return CMD_RET_FAILURE;
+}
+
+static struct cmd_tbl cmd_sub[] = {
+	U_BOOT_CMD_MKENT(sigsegv, CONFIG_SYS_MAXARGS, 1, do_sigsegv,
+			 "", ""),
+	U_BOOT_CMD_MKENT(undefined, CONFIG_SYS_MAXARGS, 1, do_undefined,
+			 "", ""),
+};
+
+static char exception_help_text[] =
+	"<ex>\n"
+	"  The following exceptions are available:\n"
+	"  undefined  - undefined instruction\n"
+	"  sigsegv    - illegal memory access\n"
+	;
+
+#include <exception.h>
--
2.28.0

[PATCH 3/3] efi_selftest: implement exception test for sandbox

[Heinrich Schuchardt]

Provide a unit test that causes an illegal instruction to occur.

The test can be run with the following commands:

    => setenv efi_selftest exception
    => bootefi selftest

This might be the output:

    Executing 'exception'
    EFI application triggers exception.
    Illegal instruction
    pc = 0x1444d016, pc_reloc = 0xffffaa078e8dd016
    UEFI image [0x0000000000000000:0xffffffffffffffff] '/\selftest'
    UEFI image [0x000000001444b000:0x0000000014451fff] pc=0x2016 '/bug.efi'
    Resetting ...

It would tell us that the exception was triggered by an instruction
0x2016 bytes after the load address of the binary with filename /bug.efi.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 lib/efi_selftest/efi_selftest_miniapp_exception.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/efi_selftest/efi_selftest_miniapp_exception.c b/lib/efi_selftest/efi_selftest_miniapp_exception.c
index 63c63d75f1..59b7e5100a 100644
--- a/lib/efi_selftest/efi_selftest_miniapp_exception.c
+++ b/lib/efi_selftest/efi_selftest_miniapp_exception.c
@@ -33,6 +33,8 @@ efi_status_t EFIAPI efi_main(efi_handle_t handle,
 	asm volatile (".word 0xe7f7defb\n");
 #elif defined(CONFIG_RISCV)
 	asm volatile (".word 0xffffffff\n");
+#elif defined(CONFIG_SANDBOX)
+	asm volatile (".word 0xffffffff\n");
 #elif defined(CONFIG_X86)
 	asm volatile (".word 0xffff\n");
 #endif
--
2.28.0

[PATCH 1/3] sandbox: add handler for exceptions

[Simon Glass]

Hi Heinrich,

On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Add a handler for SIGILL, SIGBUS, SIGSEGV.
>
> When an exception occurs print the program counter and the loaded
> UEFI binaries and reset the system.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  arch/sandbox/cpu/os.c         | 31 +++++++++++++++++++++++++++++++
>  arch/sandbox/cpu/start.c      |  4 ++++
>  arch/sandbox/lib/interrupts.c | 30 ++++++++++++++++++++++++++++++
>  include/os.h                  | 17 +++++++++++++++++
>  4 files changed, 82 insertions(+)

Generally we want to stop execution, because it indicates a bug. Then
we might want to run it again with gdb to debug it.

So I think this feature should be enabled by a flag.

Regards,
Simon

[PATCH 2/3] cmd: sandbox: implement exception command

[Simon Glass]

On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Implement the commands
>
> * exception undefined - execute an illegal instruction
> * exception sigsegv - cause a segment violation
>
> Here is a possible output:
>
>     => exception undefined
>     Illegal instruction
>     pc = 0x55eb8d0a7575, pc_reloc = 0x57575
>     Resetting ...
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/Kconfig             |  2 +-
>  cmd/Makefile            |  1 +
>  cmd/sandbox/Makefile    |  3 +++
>  cmd/sandbox/exception.c | 41 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 46 insertions(+), 1 deletion(-)
>  create mode 100644 cmd/sandbox/Makefile
>  create mode 100644 cmd/sandbox/exception.c

Reviewed-by: Simon Glass <sjg@chromium.org>

But it does need a test

[PATCH 3/3] efi_selftest: implement exception test for sandbox

[Simon Glass]

On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Provide a unit test that causes an illegal instruction to occur.
>
> The test can be run with the following commands:
>
>     => setenv efi_selftest exception
>     => bootefi selftest
>
> This might be the output:
>
>     Executing 'exception'
>     EFI application triggers exception.
>     Illegal instruction
>     pc = 0x1444d016, pc_reloc = 0xffffaa078e8dd016
>     UEFI image [0x0000000000000000:0xffffffffffffffff] '/\selftest'
>     UEFI image [0x000000001444b000:0x0000000014451fff] pc=0x2016 '/bug.efi'
>     Resetting ...
>
> It would tell us that the exception was triggered by an instruction
> 0x2016 bytes after the load address of the binary with filename /bug.efi.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_selftest/efi_selftest_miniapp_exception.c | 2 ++
>  1 file changed, 2 insertions(+)

Test for U-Boot code should be via U-Boot directly and not EFI. It is
OK to have an EFI test as well if you like, but I really don't want to
rely on the EFI complexity to test U-Boot itself.

Regards,
Simon

[PATCH 1/3] sandbox: add handler for exceptions

[Heinrich Schuchardt]

On 11.11.20 15:32, Simon Glass wrote:
> Hi Heinrich,
>
> On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Add a handler for SIGILL, SIGBUS, SIGSEGV.
>>
>> When an exception occurs print the program counter and the loaded
>> UEFI binaries and reset the system.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  arch/sandbox/cpu/os.c         | 31 +++++++++++++++++++++++++++++++
>>  arch/sandbox/cpu/start.c      |  4 ++++
>>  arch/sandbox/lib/interrupts.c | 30 ++++++++++++++++++++++++++++++
>>  include/os.h                  | 17 +++++++++++++++++
>>  4 files changed, 82 insertions(+)
>
> Generally we want to stop execution, because it indicates a bug. Then
> we might want to run it again with gdb to debug it.
>
> So I think this feature should be enabled by a flag.
>
> Regards,
> Simon
>

I understand that if I call os_launch should be customizable.

Providing the output should be helpful in any case.

Best regards

Heinrich

[PATCH 1/3] sandbox: add handler for exceptions

[Simon Glass]

Hi Heinrich,

On Wed, 11 Nov 2020 at 13:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 11.11.20 15:32, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Add a handler for SIGILL, SIGBUS, SIGSEGV.
> >>
> >> When an exception occurs print the program counter and the loaded
> >> UEFI binaries and reset the system.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  arch/sandbox/cpu/os.c         | 31 +++++++++++++++++++++++++++++++
> >>  arch/sandbox/cpu/start.c      |  4 ++++
> >>  arch/sandbox/lib/interrupts.c | 30 ++++++++++++++++++++++++++++++
> >>  include/os.h                  | 17 +++++++++++++++++
> >>  4 files changed, 82 insertions(+)
> >
> > Generally we want to stop execution, because it indicates a bug. Then
> > we might want to run it again with gdb to debug it.
> >
> > So I think this feature should be enabled by a flag.
> >
> > Regards,
> > Simon
> >
>
> I understand that if I call os_launch should be customizable.
>
> Providing the output should be helpful in any case.

I'm not sure what you mean here, but what I am saying is that by
default sandbox should crash. We don't really want the caller to have
to kill it if a test fails, etc.

Regards,
Simon