Hi Heinrich, On Sat, 20 Apr 2024 at 17:01, Heinrich Schuchardt <heinrich.schucha...@canonical.com> wrote: > > The image is not unloaded if a security violation occurs. > > If efi_set_load_options() fails, we do not free the memory allocated for > the optional data. We do not unload the image. > > If a load option is not active, we use a random value from the stack to > allocate memory for the optional data of the load option. > > * Unload the image if a security violation occurs. > * Free load_options if efi_set_load_options() fails. > * Unload the image if efi_set_load_options() fails. > * Do not allocate load_options with a random size from the stack > if the boot entry is not active.
Where is that happening? Don't we always init the size in efi_get_var() & efi_deserialize_load_option()_ which are called early? efi_get_var() especially sets size = 0 even on failures > > Fixes: 53f6a5aa8626 ("efi_loader: Replace config option for initrd loading") I think we also need a fix tag for 0ad64007feb93 as well? > Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com> > --- > lib/efi_loader/efi_bootmgr.c | 97 +++++++++---------- > test/py/tests/test_efi_secboot/test_signed.py | 28 +++--- > .../test_efi_secboot/test_signed_intca.py | 10 +- > .../tests/test_efi_secboot/test_unsigned.py | 6 +- > 4 files changed, 70 insertions(+), 71 deletions(-) > > diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c > index 4ac519228a6..ca2ebdaa32f 100644 > --- a/lib/efi_loader/efi_bootmgr.c > +++ b/lib/efi_loader/efi_bootmgr.c > @@ -613,9 +613,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t > *handle, > void *load_option; > efi_uintn_t size; > efi_status_t ret; > + u32 attributes; > > - efi_create_indexed_name(varname, sizeof(varname), "Boot", n); > + *handle = NULL; > + *load_options = NULL; > > + efi_create_indexed_name(varname, sizeof(varname), "Boot", n); > load_option = efi_get_var(varname, &efi_global_variable_guid, &size); > if (!load_option) > return EFI_LOAD_ERROR; > @@ -626,55 +629,54 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t > *handle, > goto error; > } > > - if (lo.attributes & LOAD_OPTION_ACTIVE) { > - u32 attributes; > - > - log_debug("trying to load \"%ls\" from %pD\n", lo.label, > - lo.file_path); > - > - if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) { > - /* file_path doesn't contain a device path */ > - ret = try_load_from_short_path(lo.file_path, handle); > - } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, > MSG_URI)) { > - if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) > - ret = try_load_from_uri_path( > - (struct efi_device_path_uri > *)lo.file_path, > - lo.label, handle); > - else > - ret = EFI_LOAD_ERROR; > - } else { > - ret = try_load_from_media(lo.file_path, handle); > - } > - if (ret != EFI_SUCCESS) { > - log_warning("Loading %ls '%ls' failed\n", > - varname, lo.label); > - goto error; > - } > + if (!(lo.attributes & LOAD_OPTION_ACTIVE)) { > + ret = EFI_LOAD_ERROR; > + goto error; > + } > > - attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > - EFI_VARIABLE_RUNTIME_ACCESS; > - ret = efi_set_variable_int(u"BootCurrent", > - &efi_global_variable_guid, > - attributes, sizeof(n), &n, false); > - if (ret != EFI_SUCCESS) > - goto unload; > - /* try to register load file2 for initrd's */ > - if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { > - ret = efi_initrd_register(); > - if (ret != EFI_SUCCESS) > - goto unload; > - } > + log_debug("trying to load \"%ls\" from %pD\n", lo.label, > lo.file_path); > > - log_info("Booting: %ls\n", lo.label); > + if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) { > + /* file_path doesn't contain a device path */ > + ret = try_load_from_short_path(lo.file_path, handle); > + } else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) { > + if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) > + ret = try_load_from_uri_path( > + (struct efi_device_path_uri *)lo.file_path, > + lo.label, handle); > + else > + ret = EFI_LOAD_ERROR; > } else { > - ret = EFI_LOAD_ERROR; > + ret = try_load_from_media(lo.file_path, handle); > + } > + if (ret != EFI_SUCCESS) { > + log_warning("Loading %ls '%ls' failed\n", > + varname, lo.label); > + goto error; > + } > + > + attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_RUNTIME_ACCESS; > + ret = efi_set_variable_int(u"BootCurrent", &efi_global_variable_guid, > + attributes, sizeof(n), &n, false); > + if (ret != EFI_SUCCESS) > + goto error; > + > + /* try to register load file2 for initrd's */ > + if (IS_ENABLED(CONFIG_EFI_LOAD_FILE2_INITRD)) { > + ret = efi_initrd_register(); > + if (ret != EFI_SUCCESS) > + goto error; > } > > - /* Set load options */ > + log_info("Booting: %ls\n", lo.label); > + > + /* Ignore the optional data in auto-generated boot options */ > if (size >= sizeof(efi_guid_t) && > !guidcmp(lo.optional_data, &efi_guid_bootmenu_auto_generated)) > size = 0; > > + /* Set optional data in loaded file protocol */ > if (size) { > *load_options = malloc(size); > if (!*load_options) { > @@ -683,18 +685,15 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t > *handle, > } > memcpy(*load_options, lo.optional_data, size); > ret = efi_set_load_options(*handle, size, *load_options); > - } else { > - *load_options = NULL; > + if (ret != EFI_SUCCESS) > + free(load_options); > } > > error: > - free(load_option); > - > - return ret; > - > -unload: > - if (EFI_CALL(efi_unload_image(*handle)) != EFI_SUCCESS) > + if (ret != EFI_SUCCESS && *handle && > + EFI_CALL(efi_unload_image(*handle)) != EFI_SUCCESS) > log_err("Unloading image failed\n"); > + > free(load_option); > > return ret; > diff --git a/test/py/tests/test_efi_secboot/test_signed.py > b/test/py/tests/test_efi_secboot/test_signed.py > index 2f862a259ad..5000a4ab7b6 100644 > --- a/test/py/tests/test_efi_secboot/test_signed.py > +++ b/test/py/tests/test_efi_secboot/test_signed.py > @@ -62,13 +62,13 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert('\'HELLO1\' failed' in ''.join(output)) > - assert('efi_start_image() returned: 26' in ''.join(output)) > + assert('efi_bootmgr_load() returned: 26' in ''.join(output)) > output = u_boot_console.run_command_list([ > 'efidebug boot add -b 2 HELLO2 host 0:1 /helloworld.efi -s > ""', > 'efidebug boot order 2', > 'efidebug test bootmgr']) > assert '\'HELLO2\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 2b'): > # Test Case 2b, authenticated by db > @@ -80,7 +80,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 2', > 'efidebug test bootmgr']) > assert '\'HELLO2\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > output = u_boot_console.run_command_list([ > 'efidebug boot order 1', > 'bootefi bootmgr']) > @@ -108,7 +108,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 3b'): > # Test Case 3b, rejected by dbx even if db allows > @@ -120,7 +120,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > def test_efi_signed_image_auth4(self, u_boot_console, efi_boot_env): > """ > @@ -146,7 +146,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > def test_efi_signed_image_auth5(self, u_boot_console, efi_boot_env): > """ > @@ -196,7 +196,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 5d'): > # Test Case 5d, rejected if both of signatures are revoked > @@ -208,7 +208,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > # Try rejection in reverse order. > u_boot_console.restart_uboot() > @@ -233,7 +233,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > def test_efi_signed_image_auth6(self, u_boot_console, efi_boot_env): > """ > @@ -268,7 +268,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 6c'): > # Test Case 6c, rejected by image's digest in dbx > @@ -282,7 +282,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > def test_efi_signed_image_auth7(self, u_boot_console, efi_boot_env): > """ > @@ -310,7 +310,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > # sha512 of an x509 cert in dbx > u_boot_console.restart_uboot() > @@ -333,7 +333,7 @@ class TestEfiSignedImage(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > def test_efi_signed_image_auth8(self, u_boot_console, efi_boot_env): > """ > @@ -368,4 +368,4 @@ class TestEfiSignedImage(object): > 'efidebug test bootmgr']) > assert(not 'hELLO, world!' in ''.join(output)) > assert('\'HELLO1\' failed' in ''.join(output)) > - assert('efi_start_image() returned: 26' in ''.join(output)) > + assert('efi_bootmgr_load() returned: 26' in ''.join(output)) > diff --git a/test/py/tests/test_efi_secboot/test_signed_intca.py > b/test/py/tests/test_efi_secboot/test_signed_intca.py > index 8d9a5f3e7fe..cf906205bc2 100644 > --- a/test/py/tests/test_efi_secboot/test_signed_intca.py > +++ b/test/py/tests/test_efi_secboot/test_signed_intca.py > @@ -43,7 +43,7 @@ class TestEfiSignedImageIntca(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO_a\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 1b'): > # Test Case 1b, signed and authenticated by root CA > @@ -74,7 +74,7 @@ class TestEfiSignedImageIntca(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO_abc\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 2b'): > # Test Case 2b, signed and authenticated by root CA > @@ -84,7 +84,7 @@ class TestEfiSignedImageIntca(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO_abc\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 2c'): > # Test Case 2c, signed and authenticated by root CA > @@ -122,7 +122,7 @@ class TestEfiSignedImageIntca(object): > assert 'Hello, world!' in ''.join(output) > # Or, > # assert '\'HELLO_abc\' failed' in ''.join(output) > - # assert 'efi_start_image() returned: 26' in ''.join(output) > + # assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > > with u_boot_console.log.section('Test Case 3b'): > # Test Case 3b, revoked by root CA in dbx > @@ -132,4 +132,4 @@ class TestEfiSignedImageIntca(object): > 'efidebug boot order 1', > 'efidebug test bootmgr']) > assert '\'HELLO_abc\' failed' in ''.join(output) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > diff --git a/test/py/tests/test_efi_secboot/test_unsigned.py > b/test/py/tests/test_efi_secboot/test_unsigned.py > index 7c078f220d0..b4320ae4054 100644 > --- a/test/py/tests/test_efi_secboot/test_unsigned.py > +++ b/test/py/tests/test_efi_secboot/test_unsigned.py > @@ -42,7 +42,7 @@ class TestEfiUnsignedImage(object): > output = u_boot_console.run_command_list([ > 'efidebug boot order 1', > 'efidebug test bootmgr']) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > assert 'Hello, world!' not in ''.join(output) > > def test_efi_unsigned_image_auth2(self, u_boot_console, efi_boot_env): > @@ -95,7 +95,7 @@ class TestEfiUnsignedImage(object): > output = u_boot_console.run_command_list([ > 'efidebug boot order 1', > 'efidebug test bootmgr']) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > assert 'Hello, world!' not in ''.join(output) > > with u_boot_console.log.section('Test Case 3b'): > @@ -113,5 +113,5 @@ class TestEfiUnsignedImage(object): > output = u_boot_console.run_command_list([ > 'efidebug boot order 1', > 'efidebug test bootmgr']) > - assert 'efi_start_image() returned: 26' in ''.join(output) > + assert 'efi_bootmgr_load() returned: 26' in ''.join(output) > assert 'Hello, world!' not in ''.join(output) > -- > 2.43.0 > Please adjust the commit message either in a v2 or while merging. Other than that Reviewed-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>