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

[Heinrich Schuchardt]

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.

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
>