efi_loader: improve error handling in try_load_entry()

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.

* 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.

Fixes: 53f6a5aa8626 ("efi_loader: Replace config option for initrd loading")
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
index 4ac519228a694239ffc781ab732c2ecba99ff3e2..ca2ebdaa32f76ab4f62cb930cb263cbd03b608d6 100644 (file)
--- 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 2f862a259adf2f7ed1ccdcdae228702236036424..5000a4ab7b6888b66c06ff34b6f6c610f01ed71e 100644 (file)
--- 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 8d9a5f3e7fece49dfc99c54a5a6a9ad5f9c22b82..cf906205bc206366b0d776883dd621d35cd20f1c 100644 (file)
--- 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 7c078f220d07b3fb660d11990aea9085940b897b..b4320ae40542efd5837732531265d4e9363c55c5 100644 (file)
--- 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)