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

Simon Glass sjg at chromium.org
Thu Oct 25 03:30:41 UTC 2018


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


More information about the U-Boot mailing list