[U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array

[Simon Glass]

Hi Heinrich,

On 18 October 2018 at 23:51, Heinrich Schuchardt <xypron.glpk at 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 at chromium.org>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk at 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 at 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?

> +       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