On 10/22/2018 07:49 PM, Simon Glass wrote:

Hi Heinrich,

On 18 October 2018 at 23:51, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

...

Linker generated arrays may be stored in code sections of memory that are not writable. So let's allocate setup_ok as an array at runtime.

This avoids an illegal memory access observed in the sandbox.

Reported-by: Simon Glass sjg@chromium.org Signed-off-by: Heinrich Schuchardt xypron.glpk@gmx.de

---

include/efi_selftest.h | 2 -- lib/efi_selftest/efi_selftest.c | 31 ++++++++++++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-)

Reviewed-by: Simon Glass sjg@chromium.org

Thanks for doing this!

...

diff --git a/include/efi_selftest.h b/include/efi_selftest.h index 56beac305e..49d3d6d0b4 100644 --- a/include/efi_selftest.h +++ b/include/efi_selftest.h @@ -129,7 +129,6 @@ u16 efi_st_get_key(void);

• @setup: set up the unit test
• @teardown: tear down the unit test
• @execute: execute the unit test

• • @setup_ok: setup was successful (set at runtime)

I'm not keen on the name setup_ok, which suggests a bool which would be true if it is OK.

How about setup_err or setup_ret?

...

• @on_request: test is only executed on request

*/ struct efi_unit_test { @@ -139,7 +138,6 @@ struct efi_unit_test { const struct efi_system_table *systable); int (*execute)(void); int (*teardown)(void);

•   int setup_ok;
    bool on_request;
  

};

diff --git a/lib/efi_selftest/efi_selftest.c b/lib/efi_selftest/efi_selftest.c index dd338db687..fc7866365d 100644 --- a/lib/efi_selftest/efi_selftest.c +++ b/lib/efi_selftest/efi_selftest.c @@ -18,6 +18,7 @@ static const struct efi_boot_services *boottime; static const struct efi_runtime_services *runtime; static efi_handle_t handle; static u16 reset_message[] = L"Selftest completed"; +static int *setup_ok;

/*

• Exit the boot services.

@@ -74,20 +75,20 @@ void efi_st_exit_boot_services(void) */ static int setup(struct efi_unit_test *test, unsigned int *failures) {

•   if (!test->setup) {
  

•           test->setup_ok = EFI_ST_SUCCESS;
  

•   int ret;
  

•   if (!test->setup)
            return EFI_ST_SUCCESS;
  

•   }
    efi_st_printc(EFI_LIGHTBLUE, "\nSetting up '%s'\n", test->name);
  

•   test->setup_ok = test->setup(handle, systable);
  

•   if (test->setup_ok != EFI_ST_SUCCESS) {
  

•   ret = test->setup(handle, systable);
  

•   if (ret != EFI_ST_SUCCESS) {
            efi_st_error("Setting up '%s' failed\n", test->name);
            ++*failures;
    } else {
            efi_st_printc(EFI_LIGHTGREEN,
                          "Setting up '%s' succeeded\n", test->name);
    }
  

•   return test->setup_ok;
  

•   return ret;
  

}

/* @@ -186,18 +187,20 @@ static void list_all_tests(void) void efi_st_do_tests(const u16 *testname, unsigned int phase, unsigned int steps, unsigned int *failures) {

•   int i = 0;
    struct efi_unit_test *test;
  
    for (test = ll_entry_start(struct efi_unit_test, efi_unit_test);
  

•        test < ll_entry_end(struct efi_unit_test, efi_unit_test); ++test) {
  

•        test < ll_entry_end(struct efi_unit_test, efi_unit_test);
  

•        ++test, ++i) {
            if (testname ?
                efi_st_strcmp_16_8(testname, test->name) : test->on_request)
                    continue;
            if (test->phase != phase)
                    continue;
            if (steps & EFI_ST_SETUP)
  

•                   setup(test, failures);
  

•           if (steps & EFI_ST_EXECUTE && test->setup_ok == EFI_ST_SUCCESS)
  

•                   setup_ok[i] = setup(test, failures);
  

•           if (steps & EFI_ST_EXECUTE && setup_ok[i] == EFI_ST_SUCCESS)
                    execute(test, failures);
            if (steps & EFI_ST_TEARDOWN)
                    teardown(test, failures);
  

@@ -271,6 +274,16 @@ efi_status_t EFIAPI efi_selftest(efi_handle_t image_handle, ll_entry_count(struct efi_unit_test, efi_unit_test));

•   /* Allocate buffer for setup results */
  

•   ret = boottime->allocate_pool(EFI_RUNTIME_SERVICES_DATA, sizeof(int) *
  

•                                 ll_entry_count(struct efi_unit_test,
  

•                                                efi_unit_test),
  

•                                 (void **)&setup_ok);
  

Isn't this calling the code you are trying to test and messing with the memory allocation tables? I wonder if you should allocate this in the caller or as part of the setup.

Hmmm but I suppose you don't want to, since this is sort-of an EFI app?

I would prefer to keep efi_selftest self-sufficient so that one day we can compile it as an app like we do with helloworld.efi.

Best regards

Heinrich

...

•   if (ret != EFI_SUCCESS) {
  

•           efi_st_error("Allocate pool failed\n");
  

•           return ret;
  

•   }
  

•   /* Execute boottime tests */
    efi_st_do_tests(testname, EFI_EXECUTE_BEFORE_BOOTTIME_EXIT,
                    EFI_ST_SETUP | EFI_ST_EXECUTE | EFI_ST_TEARDOWN,
  

-- 2.19.1

Regards, Simon