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

[Simon Glass]

Hi Heinrich,

On 22 October 2018 at 15:14, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> On 10/22/2018 07:49 PM, Simon Glass wrote:
>> 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?
>
> 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.

OK, but understand there are big benefits to running it within sandbox
(mainly debuggability, valgrind, etc).

Regards,
Simon