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.
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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
