* [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array
@ 2018-10-19 5:51 Heinrich Schuchardt
2018-10-22 17:49 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2018-10-19 5:51 UTC (permalink / raw)
To: u-boot
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(-)
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,
--
2.19.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array
2018-10-19 5:51 [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array Heinrich Schuchardt
@ 2018-10-22 17:49 ` Simon Glass
2018-10-22 21:14 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: Simon Glass @ 2018-10-22 17:49 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array
2018-10-22 17:49 ` Simon Glass
@ 2018-10-22 21:14 ` Heinrich Schuchardt
2018-10-25 3:30 ` Simon Glass
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2018-10-22 21:14 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array
2018-10-22 21:14 ` Heinrich Schuchardt
@ 2018-10-25 3:30 ` Simon Glass
0 siblings, 0 replies; 4+ messages in thread
From: Simon Glass @ 2018-10-25 3:30 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-10-25 3:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-19 5:51 [U-Boot] [PATCH 1/1] efi_selftest: do not write to linker generated array Heinrich Schuchardt
2018-10-22 17:49 ` Simon Glass
2018-10-22 21:14 ` Heinrich Schuchardt
2018-10-25 3:30 ` Simon Glass
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.