| Message ID | 20180618140835.195901-8-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | efi: Enable sandbox support for EFI loader | expand |
On 06/18/2018 04:08 PM, Simon Glass wrote: > We plan to run more than one EFI test. In order to avoid duplicating code, > create functions which handle preparing for running the test and cleaning > up afterwards. > > Also shorten the awfully long variable names here. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v8: None > Changes in v7: None > Changes in v6: None > Changes in v5: > - Drop call to efi_init_obj_list() which is now done in do_bootefi() > > Changes in v4: None > Changes in v3: > - Add new patch to split out test init/uninit into functions > > Changes in v2: None > > cmd/bootefi.c | 87 +++++++++++++++++++++++++++++++++++++-------------- > 1 file changed, 63 insertions(+), 24 deletions(-) > > diff --git a/cmd/bootefi.c b/cmd/bootefi.c > index f55a40dc84..2dfd297e78 100644 > --- a/cmd/bootefi.c > +++ b/cmd/bootefi.c > @@ -356,6 +356,60 @@ exit: > return ret; > } > > +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST > +/** > + * bootefi_test_prepare() - prepare to run an EFI test > + * > + * This sets things up so we can call EFI functions. This involves preparing > + * the 'gd' pointer and setting up the load ed image data structures. > + * > + * @image: Pointer to a struct which will hold the loaded image info > + * @obj: Pointer to a struct which will hold the loaded image object > + * @path: File path to the test being run (often just the test name with a > + * backslash before it > + * @test_func: Address of the test function that is being run > + * @return 0 if OK, -ve on error > + */ > +static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, > + struct efi_object *obj, > + const char *path, ulong test_func) > +{ > + memset(image, '\0', sizeof(*image)); > + memset(obj, '\0', sizeof(*obj)); It is unclear why you are adding the memsets here. > + /* Construct a dummy device path */ > + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, > + (uintptr_t)test_func, > + (uintptr_t)test_func); efi_dp_from_mem() may return NULL in case of an error. > + bootefi_image_path = efi_dp_from_file(NULL, 0, path); > + efi_setup_loaded_image(image, obj, bootefi_device_path, > + bootefi_image_path); The return code of efi_setup_loaded_image() should be checked. This function should not return 0 if the return code is not EFI_SUCCESS. Best regards Heinrich > + /* > + * gd lives in a fixed register which may get clobbered while we execute > + * the payload. So save it here and restore it on every callback entry > + */ > + efi_save_gd(); > + > + /* Transfer environment variable efi_selftest as load options */ > + set_load_options(image, "efi_selftest"); > + > + return 0; > +} > + > +/** > + * bootefi_test_finish() - finish up after running an EFI test > + * > + * @image: Pointer to a struct which holds the loaded image info > + * @obj: Pointer to a struct which holds the loaded image object > + */ > +static void bootefi_test_finish(struct efi_loaded_image *image, > + struct efi_object *obj) > +{ > + efi_restore_gd(); > + free(image->load_options); > + list_del(&obj->link); > +} > +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ > + > static int do_bootefi_bootmgr_exec(void) > { > struct efi_device_path *device_path, *file_path; > @@ -434,31 +488,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) > #endif > #ifdef CONFIG_CMD_BOOTEFI_SELFTEST > if (!strcmp(argv[1], "selftest")) { > - struct efi_loaded_image loaded_image_info = {}; > - struct efi_object loaded_image_info_obj = {}; > - > - /* Construct a dummy device path. */ > - bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, > - (uintptr_t)&efi_selftest, > - (uintptr_t)&efi_selftest); > - bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest"); > - > - efi_setup_loaded_image(&loaded_image_info, > - &loaded_image_info_obj, > - bootefi_device_path, bootefi_image_path); > - /* > - * gd lives in a fixed register which may get clobbered while we > - * execute the payload. So save it here and restore it on every > - * callback entry > - */ > - efi_save_gd(); > - /* Transfer environment variable efi_selftest as load options */ > - set_load_options(&loaded_image_info, "efi_selftest"); > + struct efi_loaded_image image; > + struct efi_object obj; > + > + if (bootefi_test_prepare(&image, &obj, "\\selftest", > + (uintptr_t)&efi_selftest)) > + return CMD_RET_FAILURE; > + > /* Execute the test */ > - r = efi_selftest(loaded_image_info_obj.handle, &systab); > - efi_restore_gd(); > - free(loaded_image_info.load_options); > - list_del(&loaded_image_info_obj.link); > + r = efi_selftest(obj.handle, &systab); > + bootefi_test_finish(&image, &obj); > return r != EFI_SUCCESS; > } else > #endif >
Hi Heinrich, On 20 June 2018 at 00:26, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/18/2018 04:08 PM, Simon Glass wrote: >> We plan to run more than one EFI test. In order to avoid duplicating code, >> create functions which handle preparing for running the test and cleaning >> up afterwards. >> >> Also shorten the awfully long variable names here. >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> Changes in v8: None >> Changes in v7: None >> Changes in v6: None >> Changes in v5: >> - Drop call to efi_init_obj_list() which is now done in do_bootefi() >> >> Changes in v4: None >> Changes in v3: >> - Add new patch to split out test init/uninit into functions >> >> Changes in v2: None >> >> cmd/bootefi.c | 87 +++++++++++++++++++++++++++++++++++++-------------- >> 1 file changed, 63 insertions(+), 24 deletions(-) >> >> diff --git a/cmd/bootefi.c b/cmd/bootefi.c >> index f55a40dc84..2dfd297e78 100644 >> --- a/cmd/bootefi.c >> +++ b/cmd/bootefi.c >> @@ -356,6 +356,60 @@ exit: >> return ret; >> } >> >> +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST >> +/** >> + * bootefi_test_prepare() - prepare to run an EFI test >> + * >> + * This sets things up so we can call EFI functions. This involves preparing >> + * the 'gd' pointer and setting up the load ed image data structures. >> + * >> + * @image: Pointer to a struct which will hold the loaded image info >> + * @obj: Pointer to a struct which will hold the loaded image object >> + * @path: File path to the test being run (often just the test name with a >> + * backslash before it >> + * @test_func: Address of the test function that is being run >> + * @return 0 if OK, -ve on error >> + */ >> +static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, >> + struct efi_object *obj, >> + const char *path, ulong test_func) >> +{ >> + memset(image, '\0', sizeof(*image)); >> + memset(obj, '\0', sizeof(*obj)); > > It is unclear why you are adding the memsets here. It's because they are passed in as pointers by callers, and otherwise would not inited. I will add a comment. > >> + /* Construct a dummy device path */ >> + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, >> + (uintptr_t)test_func, >> + (uintptr_t)test_func); > > efi_dp_from_mem() may return NULL in case of an error. > >> + bootefi_image_path = efi_dp_from_file(NULL, 0, path); >> + efi_setup_loaded_image(image, obj, bootefi_device_path, >> + bootefi_image_path); > > The return code of efi_setup_loaded_image() should be checked. This > function should not return 0 if the return code is not EFI_SUCCESS. I copied the existing code, which does not check the return value anywhere. Has this function changed, or is the current code wrong? Regards, Simon
diff --git a/cmd/bootefi.c b/cmd/bootefi.c index f55a40dc84..2dfd297e78 100644 --- a/cmd/bootefi.c +++ b/cmd/bootefi.c @@ -356,6 +356,60 @@ exit: return ret; } +#ifdef CONFIG_CMD_BOOTEFI_SELFTEST +/** + * bootefi_test_prepare() - prepare to run an EFI test + * + * This sets things up so we can call EFI functions. This involves preparing + * the 'gd' pointer and setting up the load ed image data structures. + * + * @image: Pointer to a struct which will hold the loaded image info + * @obj: Pointer to a struct which will hold the loaded image object + * @path: File path to the test being run (often just the test name with a + * backslash before it + * @test_func: Address of the test function that is being run + * @return 0 if OK, -ve on error + */ +static efi_status_t bootefi_test_prepare(struct efi_loaded_image *image, + struct efi_object *obj, + const char *path, ulong test_func) +{ + memset(image, '\0', sizeof(*image)); + memset(obj, '\0', sizeof(*obj)); + /* Construct a dummy device path */ + bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, + (uintptr_t)test_func, + (uintptr_t)test_func); + bootefi_image_path = efi_dp_from_file(NULL, 0, path); + efi_setup_loaded_image(image, obj, bootefi_device_path, + bootefi_image_path); + /* + * gd lives in a fixed register which may get clobbered while we execute + * the payload. So save it here and restore it on every callback entry + */ + efi_save_gd(); + + /* Transfer environment variable efi_selftest as load options */ + set_load_options(image, "efi_selftest"); + + return 0; +} + +/** + * bootefi_test_finish() - finish up after running an EFI test + * + * @image: Pointer to a struct which holds the loaded image info + * @obj: Pointer to a struct which holds the loaded image object + */ +static void bootefi_test_finish(struct efi_loaded_image *image, + struct efi_object *obj) +{ + efi_restore_gd(); + free(image->load_options); + list_del(&obj->link); +} +#endif /* CONFIG_CMD_BOOTEFI_SELFTEST */ + static int do_bootefi_bootmgr_exec(void) { struct efi_device_path *device_path, *file_path; @@ -434,31 +488,16 @@ static int do_bootefi(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) #endif #ifdef CONFIG_CMD_BOOTEFI_SELFTEST if (!strcmp(argv[1], "selftest")) { - struct efi_loaded_image loaded_image_info = {}; - struct efi_object loaded_image_info_obj = {}; - - /* Construct a dummy device path. */ - bootefi_device_path = efi_dp_from_mem(EFI_RESERVED_MEMORY_TYPE, - (uintptr_t)&efi_selftest, - (uintptr_t)&efi_selftest); - bootefi_image_path = efi_dp_from_file(NULL, 0, "\\selftest"); - - efi_setup_loaded_image(&loaded_image_info, - &loaded_image_info_obj, - bootefi_device_path, bootefi_image_path); - /* - * gd lives in a fixed register which may get clobbered while we - * execute the payload. So save it here and restore it on every - * callback entry - */ - efi_save_gd(); - /* Transfer environment variable efi_selftest as load options */ - set_load_options(&loaded_image_info, "efi_selftest"); + struct efi_loaded_image image; + struct efi_object obj; + + if (bootefi_test_prepare(&image, &obj, "\\selftest", + (uintptr_t)&efi_selftest)) + return CMD_RET_FAILURE; + /* Execute the test */ - r = efi_selftest(loaded_image_info_obj.handle, &systab); - efi_restore_gd(); - free(loaded_image_info.load_options); - list_del(&loaded_image_info_obj.link); + r = efi_selftest(obj.handle, &systab); + bootefi_test_finish(&image, &obj); return r != EFI_SUCCESS; } else #endif
We plan to run more than one EFI test. In order to avoid duplicating code, create functions which handle preparing for running the test and cleaning up afterwards. Also shorten the awfully long variable names here. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v8: None Changes in v7: None Changes in v6: None Changes in v5: - Drop call to efi_init_obj_list() which is now done in do_bootefi() Changes in v4: None Changes in v3: - Add new patch to split out test init/uninit into functions Changes in v2: None cmd/bootefi.c | 87 +++++++++++++++++++++++++++++++++++++-------------- 1 file changed, 63 insertions(+), 24 deletions(-)