| Message ID | 20181019055126.15380-1-xypron.glpk@gmx.de |
|---|---|
| State | Accepted |
| Commit | 4c174394caa814a185121e7b06a41dc4be5c774a |
| Headers | show |
| Series | [U-Boot,1/1] efi_selftest: do not write to linker generated array | expand |
Hi Heinrich, On 18 October 2018 at 23:51, Heinrich Schuchardt <xypron.glpk@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@chromium.org> > Signed-off-by: Heinrich Schuchardt <xypron.glpk@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@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
On 10/22/2018 07:49 PM, Simon Glass wrote: > Hi Heinrich, > > On 18 October 2018 at 23:51, Heinrich Schuchardt <xypron.glpk@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@chromium.org> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@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@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 >
Hi Heinrich, On 22 October 2018 at 15:14, Heinrich Schuchardt <xypron.glpk@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@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@chromium.org> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@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@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
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) * @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); + 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,
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@chromium.org> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- include/efi_selftest.h | 2 -- lib/efi_selftest/efi_selftest.c | 31 ++++++++++++++++++++++--------- 2 files changed, 22 insertions(+), 11 deletions(-)