Hi Heinrich,
On 22 October 2018 at 15:14, Heinrich Schuchardt <xypron.g...@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.g...@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 <s...@chromium.org>
>>> Signed-off-by: Heinrich Schuchardt <xypron.g...@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 <s...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
