From 519625977d19b7842d9b2ded8be12ed0aecbaefc Mon Sep 17 00:00:00 2001

From: Jan Janssen <medhefgo@web.de>

Date: Tue, 15 Nov 2022 18:22:38 +0100

Subject: [PATCH] boot: Rework security arch override

This simplifies the caller interface for security arch overrides by only

having to pass a validator and an optional context.

(cherry picked from commit 5489c13bae119dc5f6e65be8d7f241aa7d54c023)

Related: #2138081

---

 src/boot/efi/linux.c       |  61 ++++++++-------------

 src/boot/efi/secure-boot.c | 105 +++++++++++++++++++++++++++++--------

 src/boot/efi/secure-boot.h |  28 +++-------

 src/boot/efi/shim.c        | 104 +++++++++++-------------------------

 4 files changed, 146 insertions(+), 152 deletions(-)

diff --git a/src/boot/efi/linux.c b/src/boot/efi/linux.c

index 75b9507709..dd7eb48c8c 100644

--- a/src/boot/efi/linux.c

+++ b/src/boot/efi/linux.c

@@ -20,35 +20,26 @@

 #define STUB_PAYLOAD_GUID \

         { 0x55c5d1f8, 0x04cd, 0x46b5, { 0x8a, 0x20, 0xe5, 0x6c, 0xbb, 0x30, 0x52, 0xd0 } }

-static EFIAPI EFI_STATUS security_hook(

-                const SecurityOverride *this, uint32_t authentication_status, const EFI_DEVICE_PATH *file) {

+typedef struct {

+        const void *addr;

+        size_t len;

+        const EFI_DEVICE_PATH *device_path;

+} ValidationContext;

-        assert(this);

-        assert(this->hook == security_hook);

+static bool validate_payload(

+                const void *ctx, const EFI_DEVICE_PATH *device_path, const void *file_buffer, size_t file_size) {

-        if (file == this->payload_device_path)

-                return EFI_SUCCESS;

+        const ValidationContext *payload = ASSERT_PTR(ctx);

-        return this->original_security->FileAuthenticationState(

-                        this->original_security, authentication_status, file);

-}

-

-static EFIAPI EFI_STATUS security2_hook(

-                const SecurityOverride *this,

-                const EFI_DEVICE_PATH *device_path,

-                void *file_buffer,

-                size_t file_size,

-                BOOLEAN boot_policy) {

-

-        assert(this);

-        assert(this->hook == security2_hook);

+        if (device_path != payload->device_path)

+                return false;

-        if (file_buffer == this->payload && file_size == this->payload_len &&

-            device_path == this->payload_device_path)

-                return EFI_SUCCESS;

+        /* Security arch (1) protocol does not provide a file buffer. Instead we are supposed to fetch the payload

+         * ourselves, which is not needed as we already have everything in memory and the device paths match. */

+        if (file_buffer && (file_buffer != payload->addr || file_size != payload->len))

+                return false;

-        return this->original_security2->FileAuthentication(

-                        this->original_security2, device_path, file_buffer, file_size, boot_policy);

+        return true;

 }

 static EFI_STATUS load_image(EFI_HANDLE parent, const void *source, size_t len, EFI_HANDLE *ret_image) {

@@ -79,19 +70,13 @@ static EFI_STATUS load_image(EFI_HANDLE parent, const void *source, size_t len,

         /* We want to support unsigned kernel images as payload, which is safe to do under secure boot

          * because it is embedded in this stub loader (and since it is already running it must be trusted). */

-        SecurityOverride security_override = {

-                .hook = security_hook,

-                .payload = source,

-                .payload_len = len,

-                .payload_device_path = &payload_device_path.payload.Header,

-        }, security2_override = {

-                .hook = security2_hook,

-                .payload = source,

-                .payload_len = len,

-                .payload_device_path = &payload_device_path.payload.Header,

-        };

-

-        install_security_override(&security_override, &security2_override);

+        install_security_override(

+                        validate_payload,

+                        &(ValidationContext) {

+                                .addr = source,

+                                .len = len,

+                                .device_path = &payload_device_path.payload.Header,

+                        });

         EFI_STATUS ret = BS->LoadImage(

                         /*BootPolicy=*/false,

@@ -101,7 +86,7 @@ static EFI_STATUS load_image(EFI_HANDLE parent, const void *source, size_t len,

                         len,

                         ret_image);

-        uninstall_security_override(&security_override, &security2_override);

+        uninstall_security_override();

         return ret;

 }

diff --git a/src/boot/efi/secure-boot.c b/src/boot/efi/secure-boot.c

index 171b2c96b3..0e615c55e0 100644

--- a/src/boot/efi/secure-boot.c

+++ b/src/boot/efi/secure-boot.c

@@ -127,10 +127,60 @@ out_deallocate:

         return err;

 }

-static EFI_STATUS install_security_override_one(EFI_GUID guid, SecurityOverride *override) {

+static struct SecurityOverride {

+        /* Our own security arch instances that we register onto original_handle, thereby replacing the

+         * firmware provided instances. */

+        EFI_SECURITY_ARCH_PROTOCOL override;

+        EFI_SECURITY2_ARCH_PROTOCOL override2;

+

+        /* These are saved so we can uninstall our own instance later. */

+        EFI_HANDLE original_handle, original_handle2;

+        EFI_SECURITY_ARCH_PROTOCOL *original_security;

+        EFI_SECURITY2_ARCH_PROTOCOL *original_security2;

+

+        security_validator_t validator;

+        const void *validator_ctx;

+} security_override;

+

+static EFIAPI EFI_STATUS security_hook(

+                const EFI_SECURITY_ARCH_PROTOCOL *this,

+                uint32_t authentication_status,

+                const EFI_DEVICE_PATH *file) {

+

+        assert(security_override.validator);

+        assert(security_override.original_security);

+

+        if (security_override.validator(security_override.validator_ctx, file, NULL, 0))

+                return EFI_SUCCESS;

+

+        return security_override.original_security->FileAuthenticationState(

+                        security_override.original_security, authentication_status, file);

+}

+

+static EFIAPI EFI_STATUS security2_hook(

+                const EFI_SECURITY2_ARCH_PROTOCOL *this,

+                const EFI_DEVICE_PATH *device_path,

+                void *file_buffer,

+                size_t file_size,

+                BOOLEAN boot_policy) {

+

+        assert(security_override.validator);

+        assert(security_override.original_security2);

+

+        if (security_override.validator(security_override.validator_ctx, device_path, file_buffer, file_size))

+                return EFI_SUCCESS;

+

+        return security_override.original_security2->FileAuthentication(

+                        security_override.original_security2, device_path, file_buffer, file_size, boot_policy);

+}

+

+static EFI_STATUS install_security_override_one(

+                EFI_GUID guid, void *override, EFI_HANDLE *ret_original_handle, void **ret_original_security) {

         EFI_STATUS err;

         assert(override);

+        assert(ret_original_handle);

+        assert(ret_original_security);

         _cleanup_free_ EFI_HANDLE *handles = NULL;

         size_t n_handles = 0;

@@ -152,8 +202,8 @@ static EFI_STATUS install_security_override_one(EFI_GUID guid, SecurityOverride

         if (err != EFI_SUCCESS)

                 return log_error_status_stall(err, u"Error overriding security arch protocol: %r", err);

-        override->original = security;

-        override->original_handle = handles[0];

+        *ret_original_security = security;

+        *ret_original_handle = handles[0];

         return EFI_SUCCESS;

 }

@@ -161,35 +211,46 @@ static EFI_STATUS install_security_override_one(EFI_GUID guid, SecurityOverride

  * Specification) with the provided override instances. If not running in secure boot or the protocols are

  * not available nothing happens. The override instances are provided with the necessary info to undo this

  * in uninstall_security_override(). */

-void install_security_override(SecurityOverride *override, SecurityOverride *override2) {

-        assert(override);

-        assert(override2);

+void install_security_override(security_validator_t validator, const void *validator_ctx) {

+        assert(validator);

         if (!secure_boot_enabled())

                 return;

-        (void) install_security_override_one((EFI_GUID) EFI_SECURITY_ARCH_PROTOCOL_GUID, override);

-        (void) install_security_override_one((EFI_GUID) EFI_SECURITY2_ARCH_PROTOCOL_GUID, override2);

-}

+        security_override = (struct SecurityOverride) {

+                { .FileAuthenticationState = security_hook, },

+                { .FileAuthentication = security2_hook, },

+                .validator = validator,

+                .validator_ctx = validator_ctx,

+        };

-void uninstall_security_override(SecurityOverride *override, SecurityOverride *override2) {

-        assert(override);

-        assert(override2);

+        (void) install_security_override_one(

+                        (EFI_GUID) EFI_SECURITY_ARCH_PROTOCOL_GUID,

+                        &security_override.override,

+                        &security_override.original_handle,

+                        (void **) &security_override.original_security);

+        (void) install_security_override_one(

+                        (EFI_GUID) EFI_SECURITY2_ARCH_PROTOCOL_GUID,

+                        &security_override.override2,

+                        &security_override.original_handle2,

+                        (void **) &security_override.original_security2);

+}

+void uninstall_security_override(void) {

         /* We use assert_se here to guarantee the system is not in a weird state in the unlikely case of an

          * error restoring the original protocols. */

-        if (override->original_handle)

+        if (security_override.original_handle)

                 assert_se(BS->ReinstallProtocolInterface(

-                                          override->original_handle,

-                                          &(EFI_GUID) EFI_SECURITY_ARCH_PROTOCOL_GUID,

-                                          override,

-                                          override->original) == EFI_SUCCESS);

+                                security_override.original_handle,

+                                &(EFI_GUID) EFI_SECURITY_ARCH_PROTOCOL_GUID,

+                                &security_override.override,

+                                security_override.original_security) == EFI_SUCCESS);

-        if (override2->original_handle)

+        if (security_override.original_handle2)

                 assert_se(BS->ReinstallProtocolInterface(

-                                          override2->original_handle,

-                                          &(EFI_GUID) EFI_SECURITY2_ARCH_PROTOCOL_GUID,

-                                          override2,

-                                          override2->original) == EFI_SUCCESS);

+                                security_override.original_handle2,

+                                &(EFI_GUID) EFI_SECURITY2_ARCH_PROTOCOL_GUID,

+                                &security_override.override2,

+                                security_override.original_security2) == EFI_SUCCESS);

 }

diff --git a/src/boot/efi/secure-boot.h b/src/boot/efi/secure-boot.h

index 91b6770edb..e98de81c2a 100644

--- a/src/boot/efi/secure-boot.h

+++ b/src/boot/efi/secure-boot.h

@@ -17,23 +17,11 @@ SecureBootMode secure_boot_mode(void);

 EFI_STATUS secure_boot_enroll_at(EFI_FILE *root_dir, const char16_t *path);

-typedef struct {

-        void *hook;

-

-        /* End of EFI_SECURITY_ARCH(2)_PROTOCOL. The rest is our own protocol instance data. */

-

-        EFI_HANDLE original_handle;

-        union {

-                void *original;

-                EFI_SECURITY_ARCH_PROTOCOL *original_security;

-                EFI_SECURITY2_ARCH_PROTOCOL *original_security2;

-        };

-

-        /* Used by the stub to identify the embedded image. */

-        const void *payload;

-        size_t payload_len;

-        const EFI_DEVICE_PATH *payload_device_path;

-} SecurityOverride;

-

-void install_security_override(SecurityOverride *override, SecurityOverride *override2);

-void uninstall_security_override(SecurityOverride *override, SecurityOverride *override2);

+typedef bool (*security_validator_t)(

+                const void *ctx,

+                const EFI_DEVICE_PATH *device_path,

+                const void *file_buffer,

+                size_t file_size);

+

+void install_security_override(security_validator_t validator, const void *validator_ctx);

+void uninstall_security_override(void);

diff --git a/src/boot/efi/shim.c b/src/boot/efi/shim.c

index 3ae058cb84..ac224336bc 100644

--- a/src/boot/efi/shim.c

+++ b/src/boot/efi/shim.c

@@ -23,7 +23,7 @@

 #endif

 struct ShimLock {

-        EFI_STATUS __sysv_abi__ (*shim_verify) (void *buffer, uint32_t size);

+        EFI_STATUS __sysv_abi__ (*shim_verify) (const void *buffer, uint32_t size);

         /* context is actually a struct for the PE header, but it isn't needed so void is sufficient just do define the interface

          * see shim.c/shim.h and PeHeader.h in the github shim repo */

@@ -41,79 +41,45 @@ bool shim_loaded(void) {

         return BS->LocateProtocol((EFI_GUID*) SHIM_LOCK_GUID, NULL, (void**) &shim_lock) == EFI_SUCCESS;

 }

-static bool shim_validate(void *data, uint32_t size) {

-        struct ShimLock *shim_lock;

-

-        if (!data)

-                return false;

-

-        if (BS->LocateProtocol((EFI_GUID*) SHIM_LOCK_GUID, NULL, (void**) &shim_lock) != EFI_SUCCESS)

-                return false;

-

-        if (!shim_lock)

-                return false;

-

-        return shim_lock->shim_verify(data, size) == EFI_SUCCESS;

-}

-

-static EFIAPI EFI_STATUS security2_hook(

-                const SecurityOverride *this,

-                const EFI_DEVICE_PATH *device_path,

-                void *file_buffer,

-                UINTN file_size,

-                BOOLEAN boot_policy) {

-

-        assert(this);

-        assert(this->hook == security2_hook);

-

-        if (shim_validate(file_buffer, file_size))

-                return EFI_SUCCESS;

-

-        return this->original_security2->FileAuthentication(

-                        this->original_security2, device_path, file_buffer, file_size, boot_policy);

-}

-

-static EFIAPI EFI_STATUS security_hook(

-                const SecurityOverride *this,

-                uint32_t authentication_status,

-                const EFI_DEVICE_PATH *device_path) {

+static bool shim_validate(

+                const void *ctx, const EFI_DEVICE_PATH *device_path, const void *file_buffer, size_t file_size) {

         EFI_STATUS err;

+        _cleanup_free_ char *file_buffer_owned = NULL;

-        assert(this);

-        assert(this->hook == security_hook);

+        if (!file_buffer) {

+                if (!device_path)

+                        return false;

-        if (!device_path)

-                return this->original_security->FileAuthenticationState(

-                                this->original_security, authentication_status, device_path);

+                EFI_HANDLE device_handle;

+                EFI_DEVICE_PATH *file_dp = (EFI_DEVICE_PATH *) device_path;

+                err = BS->LocateDevicePath(&FileSystemProtocol, &file_dp, &device_handle);

+                if (err != EFI_SUCCESS)

+                        return false;

-        EFI_HANDLE device_handle;

-        EFI_DEVICE_PATH *dp = (EFI_DEVICE_PATH *) device_path;

-        err = BS->LocateDevicePath(&FileSystemProtocol, &dp, &device_handle);

-        if (err != EFI_SUCCESS)

-                return err;

+                _cleanup_(file_closep) EFI_FILE *root = NULL;

+                err = open_volume(device_handle, &root);

+                if (err != EFI_SUCCESS)

+                        return false;

-        _cleanup_(file_closep) EFI_FILE *root = NULL;

-        err = open_volume(device_handle, &root);

-        if (err != EFI_SUCCESS)

-                return err;

+                _cleanup_free_ char16_t *dp_str = NULL;

+                err = device_path_to_str(file_dp, &dp_str);

+                if (err != EFI_SUCCESS)

+                        return false;

-        _cleanup_free_ char16_t *dp_str = NULL;

-        err = device_path_to_str(dp, &dp_str);

-        if (err != EFI_SUCCESS)

-                return err;

+                err = file_read(root, dp_str, 0, 0, &file_buffer_owned, &file_size);

+                if (err != EFI_SUCCESS)

+                        return false;

-        char *file_buffer;

-        size_t file_size;

-        err = file_read(root, dp_str, 0, 0, &file_buffer, &file_size);

-        if (err != EFI_SUCCESS)

-                return err;

+                file_buffer = file_buffer_owned;

+        }

-        if (shim_validate(file_buffer, file_size))

-                return EFI_SUCCESS;

+        struct ShimLock *shim_lock;

+        err = BS->LocateProtocol((EFI_GUID *) SHIM_LOCK_GUID, NULL, (void **) &shim_lock);

+        if (err != EFI_SUCCESS)

+                return false;

-        return this->original_security->FileAuthenticationState(

-                        this->original_security, authentication_status, device_path);

+        return shim_lock->shim_verify(file_buffer, file_size) == EFI_SUCCESS;

 }

 EFI_STATUS shim_load_image(EFI_HANDLE parent, const EFI_DEVICE_PATH *device_path, EFI_HANDLE *ret_image) {

@@ -122,20 +88,14 @@ EFI_STATUS shim_load_image(EFI_HANDLE parent, const EFI_DEVICE_PATH *device_path

         bool have_shim = shim_loaded();

-        SecurityOverride security_override = {

-                .hook = security_hook,

-        }, security2_override = {

-                .hook = security2_hook,

-        };

-

         if (have_shim)

-                install_security_override(&security_override, &security2_override);

+                install_security_override(shim_validate, NULL);

         EFI_STATUS ret = BS->LoadImage(

                         /*BootPolicy=*/false, parent, (EFI_DEVICE_PATH *) device_path, NULL, 0, ret_image);

         if (have_shim)

-                uninstall_security_override(&security_override, &security2_override);

+                uninstall_security_override();

         return ret;

 }