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?
> + 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
