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

[Simon Glass]

Hi Heinrich,

On Wed, 11 Nov 2020 at 13:47, Heinrich Schuchardt  wrote:
>
> On 11.11.20 15:32, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt  
> > 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 
> >> ---
> >>  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

Re: [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  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 
>> ---
>>  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

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

[Simon Glass]

Hi Heinrich,

On Tue, 10 Nov 2020 at 16:09, Heinrich Schuchardt  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 
> ---
>  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 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 
---
 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 
 #include 
 #include 
@@ -15,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -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(_mask);
+   act.sa_flags = SA_SIGINFO;
+   if (sigaction(SIGILL, , NULL) ||
+   sigaction(SIGBUS, , NULL) ||
+   sigaction(SIGSEGV, , NULL))
+   return -1;
+   return 0;
+}
+
 /* Put tty into raw mode so  and  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 
+#include 
 #include 
+#include 
+#include 
+#include 
+
+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