* [PATCH 01/16] hw/uefi: add include/hw/uefi/var-service-api.h
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-16 12:46 ` Laszlo Ersek
2023-11-15 15:12 ` [PATCH 02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h Gerd Hoffmann
` (15 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This file defines the register interface of the uefi-vars device.
It's only a handful of registers: magic value, command and status
registers, location and size of the communication buffer.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/hw/uefi/var-service-api.h | 40 +++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)
create mode 100644 include/hw/uefi/var-service-api.h
diff --git a/include/hw/uefi/var-service-api.h b/include/hw/uefi/var-service-api.h
new file mode 100644
index 000000000000..37fdab32741f
--- /dev/null
+++ b/include/hw/uefi/var-service-api.h
@@ -0,0 +1,40 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi-vars device - API of the virtual device for guest/host communication.
+ */
+#ifndef QEMU_UEFI_VAR_SERVICE_API_H
+#define QEMU_UEFI_VAR_SERVICE_API_H
+
+
+/* isa: io range */
+#define UEFI_VARS_IO_BASE 0x520
+
+/* sysbus: fdt node path */
+#define UEFI_VARS_FDT_NODE "qemu-uefi-vars"
+#define UEFI_VARS_FDT_COMPAT "qemu,uefi-vars"
+
+/* registers */
+#define UEFI_VARS_REG_MAGIC 0x00 /* 16 bit */
+#define UEFI_VARS_REG_CMD_STS 0x02 /* 16 bit */
+#define UEFI_VARS_REG_BUFFER_SIZE 0x04 /* 32 bit */
+#define UEFI_VARS_REG_BUFFER_ADDR_LO 0x08 /* 32 bit */
+#define UEFI_VARS_REG_BUFFER_ADDR_HI 0x0c /* 32 bit */
+#define UEFI_VARS_REGS_SIZE 0x10
+
+/* magic value */
+#define UEFI_VARS_MAGIC_VALUE 0xef1
+
+/* command values */
+#define UEFI_VARS_CMD_RESET 0x01
+#define UEFI_VARS_CMD_MM 0x02
+
+/* status values */
+#define UEFI_VARS_STS_SUCCESS 0x00
+#define UEFI_VARS_STS_BUSY 0x01
+#define UEFI_VARS_STS_ERR_UNKNOWN 0x10
+#define UEFI_VARS_STS_ERR_NOT_SUPPORTED 0x11
+#define UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE 0x12
+
+
+#endif /* QEMU_UEFI_VAR_SERVICE_API_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 01/16] hw/uefi: add include/hw/uefi/var-service-api.h
2023-11-15 15:12 ` [PATCH 01/16] hw/uefi: add include/hw/uefi/var-service-api.h Gerd Hoffmann
@ 2023-11-16 12:46 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-16 12:46 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau, Daniel P. Berrangé,
graf, Philippe Mathieu-Daudé, Markus Armbruster
On 11/15/23 16:12, Gerd Hoffmann wrote:
> This file defines the register interface of the uefi-vars device.
> It's only a handful of registers: magic value, command and status
> registers, location and size of the communication buffer.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/hw/uefi/var-service-api.h | 40 +++++++++++++++++++++++++++++++
> 1 file changed, 40 insertions(+)
> create mode 100644 include/hw/uefi/var-service-api.h
>
> diff --git a/include/hw/uefi/var-service-api.h b/include/hw/uefi/var-service-api.h
> new file mode 100644
> index 000000000000..37fdab32741f
> --- /dev/null
> +++ b/include/hw/uefi/var-service-api.h
> @@ -0,0 +1,40 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi-vars device - API of the virtual device for guest/host communication.
> + */
> +#ifndef QEMU_UEFI_VAR_SERVICE_API_H
> +#define QEMU_UEFI_VAR_SERVICE_API_H
> +
> +
> +/* isa: io range */
> +#define UEFI_VARS_IO_BASE 0x520
> +
> +/* sysbus: fdt node path */
> +#define UEFI_VARS_FDT_NODE "qemu-uefi-vars"
> +#define UEFI_VARS_FDT_COMPAT "qemu,uefi-vars"
> +
> +/* registers */
> +#define UEFI_VARS_REG_MAGIC 0x00 /* 16 bit */
> +#define UEFI_VARS_REG_CMD_STS 0x02 /* 16 bit */
> +#define UEFI_VARS_REG_BUFFER_SIZE 0x04 /* 32 bit */
> +#define UEFI_VARS_REG_BUFFER_ADDR_LO 0x08 /* 32 bit */
> +#define UEFI_VARS_REG_BUFFER_ADDR_HI 0x0c /* 32 bit */
> +#define UEFI_VARS_REGS_SIZE 0x10
> +
> +/* magic value */
> +#define UEFI_VARS_MAGIC_VALUE 0xef1
> +
> +/* command values */
> +#define UEFI_VARS_CMD_RESET 0x01
> +#define UEFI_VARS_CMD_MM 0x02
> +
> +/* status values */
> +#define UEFI_VARS_STS_SUCCESS 0x00
> +#define UEFI_VARS_STS_BUSY 0x01
> +#define UEFI_VARS_STS_ERR_UNKNOWN 0x10
> +#define UEFI_VARS_STS_ERR_NOT_SUPPORTED 0x11
> +#define UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE 0x12
> +
> +
> +#endif /* QEMU_UEFI_VAR_SERVICE_API_H */
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 01/16] hw/uefi: add include/hw/uefi/var-service-api.h Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-16 15:23 ` Laszlo Ersek
2023-11-15 15:12 ` [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h Gerd Hoffmann
` (14 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
A bunch of #defines and structs copied over from edk2,
mostly needed to decode and encode the messages in the
communication buffer.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/hw/uefi/var-service-edk2.h | 184 +++++++++++++++++++++++++++++
1 file changed, 184 insertions(+)
create mode 100644 include/hw/uefi/var-service-edk2.h
diff --git a/include/hw/uefi/var-service-edk2.h b/include/hw/uefi/var-service-edk2.h
new file mode 100644
index 000000000000..354b74d1d71c
--- /dev/null
+++ b/include/hw/uefi/var-service-edk2.h
@@ -0,0 +1,184 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi-vars device - structs and defines from edk2
+ *
+ * Note: The edk2 UINTN type has been mapped to uint64_t,
+ * so the structs are compatible with 64bit edk2 builds.
+ */
+#ifndef QEMU_UEFI_VAR_SERVICE_EDK2_H
+#define QEMU_UEFI_VAR_SERVICE_EDK2_H
+
+#include "qemu/uuid.h"
+
+#define MAX_BIT 0x8000000000000000ULL
+#define ENCODE_ERROR(StatusCode) (MAX_BIT | (StatusCode))
+#define EFI_SUCCESS 0
+#define EFI_INVALID_PARAMETER ENCODE_ERROR(2)
+#define EFI_UNSUPPORTED ENCODE_ERROR(3)
+#define EFI_BAD_BUFFER_SIZE ENCODE_ERROR(4)
+#define EFI_BUFFER_TOO_SMALL ENCODE_ERROR(5)
+#define EFI_WRITE_PROTECTED ENCODE_ERROR(8)
+#define EFI_OUT_OF_RESOURCES ENCODE_ERROR(9)
+#define EFI_NOT_FOUND ENCODE_ERROR(14)
+#define EFI_ACCESS_DENIED ENCODE_ERROR(15)
+#define EFI_ALREADY_STARTED ENCODE_ERROR(20)
+
+#define EFI_VARIABLE_NON_VOLATILE 0x01
+#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x02
+#define EFI_VARIABLE_RUNTIME_ACCESS 0x04
+#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x08
+#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x10 // deprecated
+#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x20
+#define EFI_VARIABLE_APPEND_WRITE 0x40
+
+/* SecureBootEnable */
+#define SECURE_BOOT_ENABLE 1
+#define SECURE_BOOT_DISABLE 0
+
+/* SecureBoot */
+#define SECURE_BOOT_MODE_ENABLE 1
+#define SECURE_BOOT_MODE_DISABLE 0
+
+/* CustomMode */
+#define CUSTOM_SECURE_BOOT_MODE 1
+#define STANDARD_SECURE_BOOT_MODE 0
+
+/* SetupMode */
+#define SETUP_MODE 1
+#define USER_MODE 0
+
+typedef uint64_t efi_status;
+typedef struct mm_header mm_header;
+
+/* EFI_MM_COMMUNICATE_HEADER */
+struct mm_header {
+ QemuUUID guid;
+ uint64_t length;
+};
+
+/* --- EfiSmmVariableProtocol ---------------------------------------- */
+
+#define SMM_VARIABLE_FUNCTION_GET_VARIABLE 1
+#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME 2
+#define SMM_VARIABLE_FUNCTION_SET_VARIABLE 3
+#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO 4
+#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT 5
+#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE 6
+#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE 8
+#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE 11
+
+typedef struct mm_variable mm_variable;
+typedef struct mm_variable_access mm_variable_access;
+typedef struct mm_next_variable mm_next_variable;
+typedef struct mm_next_variable mm_lock_variable;
+typedef struct mm_variable_info mm_variable_info;
+typedef struct mm_get_payload_size mm_get_payload_size;
+
+/* SMM_VARIABLE_COMMUNICATE_HEADER */
+struct mm_variable {
+ uint64_t function;
+ uint64_t status;
+};
+
+/* SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE */
+struct QEMU_PACKED mm_variable_access {
+ QemuUUID guid;
+ uint64_t data_size;
+ uint64_t name_size;
+ uint32_t attributes;
+ /* Name */
+ /* Data */
+};
+
+/* SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME */
+struct mm_next_variable {
+ QemuUUID guid;
+ uint64_t name_size;
+ /* Name */
+};
+
+/* SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO */
+struct QEMU_PACKED mm_variable_info {
+ uint64_t max_storage_size;
+ uint64_t free_storage_size;
+ uint64_t max_variable_size;
+ uint32_t attributes;
+};
+
+/* SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE */
+struct mm_get_payload_size {
+ uint64_t payload_size;
+};
+
+/* --- VarCheckPolicyLibMmiHandler ----------------------------------- */
+
+#define VAR_CHECK_POLICY_COMMAND_DISABLE 0x01
+#define VAR_CHECK_POLICY_COMMAND_IS_ENABLED 0x02
+#define VAR_CHECK_POLICY_COMMAND_REGISTER 0x03
+#define VAR_CHECK_POLICY_COMMAND_DUMP 0x04
+#define VAR_CHECK_POLICY_COMMAND_LOCK 0x05
+
+typedef struct mm_check_policy mm_check_policy;
+typedef struct mm_check_policy_is_enabled mm_check_policy_is_enabled;
+typedef struct mm_check_policy_dump_params mm_check_policy_dump_params;
+
+/* VAR_CHECK_POLICY_COMM_HEADER */
+struct QEMU_PACKED mm_check_policy {
+ uint32_t signature;
+ uint32_t revision;
+ uint32_t command;
+ uint64_t result;
+};
+
+/* VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS */
+struct QEMU_PACKED mm_check_policy_is_enabled {
+ uint8_t state;
+};
+
+/* VAR_CHECK_POLICY_COMM_DUMP_PARAMS */
+struct QEMU_PACKED mm_check_policy_dump_params {
+ uint32_t page_requested;
+ uint32_t total_size;
+ uint32_t page_size;
+ uint8_t has_more;
+};
+
+/* --- Edk2VariablePolicyProtocol ------------------------------------ */
+
+#define VARIABLE_POLICY_ENTRY_REVISION 0x00010000
+
+#define VARIABLE_POLICY_TYPE_NO_LOCK 0
+#define VARIABLE_POLICY_TYPE_LOCK_NOW 1
+#define VARIABLE_POLICY_TYPE_LOCK_ON_CREATE 2
+#define VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE 3
+
+typedef struct variable_policy_entry variable_policy_entry;
+typedef struct variable_lock_on_var_state variable_lock_on_var_state;
+
+/* VARIABLE_POLICY_ENTRY */
+struct variable_policy_entry {
+ uint32_t version;
+ uint16_t size;
+ uint16_t offset_to_name;
+ QemuUUID namespace;
+ uint32_t min_size;
+ uint32_t max_size;
+ uint32_t attributes_must_have;
+ uint32_t attributes_cant_have;
+ uint8_t lock_policy_type;
+ uint8_t padding[3];
+ /* LockPolicy */
+ /* Name */
+};
+
+/* VARIABLE_LOCK_ON_VAR_STATE_POLICY */
+struct variable_lock_on_var_state {
+ QemuUUID namespace;
+ uint8_t value;
+ uint8_t padding;
+ /* Name */
+};
+
+
+#endif /* QEMU_UEFI_VAR_SERVICE_EDK2_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h
2023-11-15 15:12 ` [PATCH 02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h Gerd Hoffmann
@ 2023-11-16 15:23 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-16 15:23 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau, Daniel P. Berrangé,
graf, Philippe Mathieu-Daudé, Markus Armbruster
On 11/15/23 16:12, Gerd Hoffmann wrote:
> A bunch of #defines and structs copied over from edk2,
> mostly needed to decode and encode the messages in the
> communication buffer.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/hw/uefi/var-service-edk2.h | 184 +++++++++++++++++++++++++++++
> 1 file changed, 184 insertions(+)
> create mode 100644 include/hw/uefi/var-service-edk2.h
>
> diff --git a/include/hw/uefi/var-service-edk2.h b/include/hw/uefi/var-service-edk2.h
> new file mode 100644
> index 000000000000..354b74d1d71c
> --- /dev/null
> +++ b/include/hw/uefi/var-service-edk2.h
> @@ -0,0 +1,184 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi-vars device - structs and defines from edk2
> + *
> + * Note: The edk2 UINTN type has been mapped to uint64_t,
> + * so the structs are compatible with 64bit edk2 builds.
(1) What is the failure mode if the guest runs a 32-bit DXE phase and
tries to access this device?
> + */
> +#ifndef QEMU_UEFI_VAR_SERVICE_EDK2_H
> +#define QEMU_UEFI_VAR_SERVICE_EDK2_H
> +
> +#include "qemu/uuid.h"
> +
> +#define MAX_BIT 0x8000000000000000ULL
> +#define ENCODE_ERROR(StatusCode) (MAX_BIT | (StatusCode))
> +#define EFI_SUCCESS 0
(2) Probably better to make this 0ULL, so that its type is consistent
with that of the error codes.
(3) BTW, these error codes are not edk2-specific; they are from the UEFI
spec (Appendix D, "Status Codes"). I'm mentioning it because it might
clarify the commit message.
> +#define EFI_INVALID_PARAMETER ENCODE_ERROR(2)
> +#define EFI_UNSUPPORTED ENCODE_ERROR(3)
> +#define EFI_BAD_BUFFER_SIZE ENCODE_ERROR(4)
> +#define EFI_BUFFER_TOO_SMALL ENCODE_ERROR(5)
(4) any particular reason for skipping NOT_READY (6) and DEVICE_ERROR (7)?
(If this file only defines status codes that the code actually uses,
that's best!)
> +#define EFI_WRITE_PROTECTED ENCODE_ERROR(8)
> +#define EFI_OUT_OF_RESOURCES ENCODE_ERROR(9)
> +#define EFI_NOT_FOUND ENCODE_ERROR(14)
> +#define EFI_ACCESS_DENIED ENCODE_ERROR(15)
> +#define EFI_ALREADY_STARTED ENCODE_ERROR(20)
> +
> +#define EFI_VARIABLE_NON_VOLATILE 0x01
> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x02
> +#define EFI_VARIABLE_RUNTIME_ACCESS 0x04
> +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x08
> +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x10 // deprecated
> +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x20
> +#define EFI_VARIABLE_APPEND_WRITE 0x40
> +
> +/* SecureBootEnable */
(5) L"SecureBootEnable"
(6) mentioning the variable namespace GUID might be worthwhile as well
(gEfiSecureBootEnableDisableGuid -- F0A30BC7-AF08-4556-99C4-001009C93A44)
(7) this variable is an edk2 extension indeed; might be worth mentioning
> +#define SECURE_BOOT_ENABLE 1
> +#define SECURE_BOOT_DISABLE 0
> +
> +/* SecureBoot */
(8) L"SecureBoot"
(9) this one is standard, so the namespace GUID is not important to
mention -- but maybe mention that it is standard
> +#define SECURE_BOOT_MODE_ENABLE 1
> +#define SECURE_BOOT_MODE_DISABLE 0
> +
> +/* CustomMode */
the usual comments:
(10) L"CustomMode"
(11) GUID: gEfiCustomModeEnableGuid -- C076EC0C-7028-4399-A072-71EE5C448B9F
(12) edk2 extension
> +#define CUSTOM_SECURE_BOOT_MODE 1
> +#define STANDARD_SECURE_BOOT_MODE 0
> +
> +/* SetupMode */
(13) L"SetupMode"
(14) standard
> +#define SETUP_MODE 1
> +#define USER_MODE 0
> +
> +typedef uint64_t efi_status;
> +typedef struct mm_header mm_header;
> +
> +/* EFI_MM_COMMUNICATE_HEADER */
> +struct mm_header {
> + QemuUUID guid;
> + uint64_t length;
> +};
(15) QEMU_PACKED
> +
> +/* --- EfiSmmVariableProtocol ---------------------------------------- */
(16) this is a bit too cryptic like this; what we mean here is that
mm_header.guid is gEfiSmmVariableProtocolGuid
(ED32D533-99E6-4209-9CC0-2D72CDD998A7) for the following functions
(17) do you want to define SMM_VARIABLE_COMMUNICATE_HEADER as well?
(because the function codes make sense inside that header) -- ah wait,
those are below; OK
(18) slight inconsistency: the function macros are named SMM_*, but the
types are named mm_*.
However... that inconsistency seems to be there in edk2 to as well; we
don't have (for example) "MM_VARIABLE_FUNCTION_GET_VARIABLE"
> +
> +#define SMM_VARIABLE_FUNCTION_GET_VARIABLE 1
> +#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME 2
> +#define SMM_VARIABLE_FUNCTION_SET_VARIABLE 3
> +#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO 4
> +#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT 5
> +#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE 6
> +#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE 8
> +#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE 11
> +
> +typedef struct mm_variable mm_variable;
> +typedef struct mm_variable_access mm_variable_access;
> +typedef struct mm_next_variable mm_next_variable;
> +typedef struct mm_next_variable mm_lock_variable;
> +typedef struct mm_variable_info mm_variable_info;
> +typedef struct mm_get_payload_size mm_get_payload_size;
> +
> +/* SMM_VARIABLE_COMMUNICATE_HEADER */
> +struct mm_variable {
> + uint64_t function;
> + uint64_t status;
> +};
(19) you have a typedef for efi_status above; no need to decode it to
uint64_t here (for the status field)
(20) This too should be QEMU_PACKED. (Will not make a difference in
practice, but this is wire protocol, so best be clear.)
BTW I'm just noticing that SMM_VARIABLE_COMMUNICATE_HEADER in edk2's
"MdeModulePkg/Include/Guid/SmmVariableCommon.h" is *not* marked packed.
That's a bug in edk2, of course (documentation bug).
(21) a comment about the general request structure would be helpful:
mm_header + mm_variable + function payload (any).
> +
> +/* SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE */
(22) worth mentioning functions 1 and 3?
> +struct QEMU_PACKED mm_variable_access {
I can see you packed this! :)
> + QemuUUID guid;
> + uint64_t data_size;
correct, but we'll have to be careful with the range checks / buffer
size checks later, because UEFI_VARS_REG_BUFFER_SIZE is only 32-bit!
> + uint64_t name_size;
> + uint32_t attributes;
> + /* Name */
> + /* Data */
> +};
> +
> +/* SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME */
(23) worth mentioning function 2, or is that obvious from the name?
> +struct mm_next_variable {
> + QemuUUID guid;
> + uint64_t name_size;
> + /* Name */
> +};
(24) should be packed
> +
> +/* SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO */
> +struct QEMU_PACKED mm_variable_info {
> + uint64_t max_storage_size;
> + uint64_t free_storage_size;
> + uint64_t max_variable_size;
> + uint32_t attributes;
> +};
(25) mention function 4?
> +
> +/* SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE */
(26) mention function 11?
> +struct mm_get_payload_size {
> + uint64_t payload_size;
> +};
(27) should be packed (just for documentation / exluding trailing padding)
> +
> +/* --- VarCheckPolicyLibMmiHandler ----------------------------------- */
(28) Please be clearer: this means that we place GUID
DA1B0D11-D1A7-46C4-9DC9-F3714875C6EB into mm_header.guid.
(29) Also record that the generic structure is mm_header +
mm_check_policy + command payload (if any).
> +
> +#define VAR_CHECK_POLICY_COMMAND_DISABLE 0x01
> +#define VAR_CHECK_POLICY_COMMAND_IS_ENABLED 0x02
> +#define VAR_CHECK_POLICY_COMMAND_REGISTER 0x03
> +#define VAR_CHECK_POLICY_COMMAND_DUMP 0x04
> +#define VAR_CHECK_POLICY_COMMAND_LOCK 0x05
> +
> +typedef struct mm_check_policy mm_check_policy;
> +typedef struct mm_check_policy_is_enabled mm_check_policy_is_enabled;
> +typedef struct mm_check_policy_dump_params mm_check_policy_dump_params;
> +
> +/* VAR_CHECK_POLICY_COMM_HEADER */
> +struct QEMU_PACKED mm_check_policy {
> + uint32_t signature;
> + uint32_t revision;
> + uint32_t command;
> + uint64_t result;
(30) field "result" should have "efi_status"
(31) define the macros for (a) signature (VCPC) and (b) revision (1)?
> +};
> +
> +/* VAR_CHECK_POLICY_COMM_IS_ENABLED_PARAMS */
(32) mention command 2? (Just guessing)
> +struct QEMU_PACKED mm_check_policy_is_enabled {
> + uint8_t state;
> +};
(33) given that this is a "BOOLEAN" in the original, we might want to
introduce a typedef "edk2_boolean" for uint8_t. Just an idea.
> +
> +/* VAR_CHECK_POLICY_COMM_DUMP_PARAMS */
(34) mention command 4?
> +struct QEMU_PACKED mm_check_policy_dump_params {
> + uint32_t page_requested;
> + uint32_t total_size;
> + uint32_t page_size;
> + uint8_t has_more;
(35) consider updating the last field's type to "edk2_boolean".
> +};
> +
> +/* --- Edk2VariablePolicyProtocol ------------------------------------ */
(36) the proper spelling is "EdkiiVariablePolicyProtocol" (note: roman
numeral "ii" rather than arabic numeral 2)
(37) Mentioning this protocol here is confusing, IMO. Please correct me
if I'm wrong, but the protocol appears to be used between DXE
components, and not on the DXE<->SMM boundary.
Instead, we need the following ABIs as payloads for
VAR_CHECK_POLICY_COMMAND_REGISTER.
Therefore I would remove the EdkiiVariablePolicyProtocol mention above
altogether, and instead clarify that these are for:
- mm_header + mm_check_policy (command 3) + variable_policy_entry,
or
- mm_header + mm_check_policy (command 3) +
variable_policy_entry (policy 3) + variable_lock_on_var_state
> +
> +#define VARIABLE_POLICY_ENTRY_REVISION 0x00010000
> +
> +#define VARIABLE_POLICY_TYPE_NO_LOCK 0
> +#define VARIABLE_POLICY_TYPE_LOCK_NOW 1
> +#define VARIABLE_POLICY_TYPE_LOCK_ON_CREATE 2
> +#define VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE 3
> +
> +typedef struct variable_policy_entry variable_policy_entry;
> +typedef struct variable_lock_on_var_state variable_lock_on_var_state;
> +
> +/* VARIABLE_POLICY_ENTRY */
> +struct variable_policy_entry {
(38) should be packed
> + uint32_t version;
> + uint16_t size;
> + uint16_t offset_to_name;
> + QemuUUID namespace;
> + uint32_t min_size;
> + uint32_t max_size;
(39) also define the macros for "no min size" and "no max size"?
> + uint32_t attributes_must_have;
> + uint32_t attributes_cant_have;
(40) define the macros that invalidate these fields?
> + uint8_t lock_policy_type;
> + uint8_t padding[3];
> + /* LockPolicy */
> + /* Name */
> +};
> +
> +/* VARIABLE_LOCK_ON_VAR_STATE_POLICY */
> +struct variable_lock_on_var_state {
(41) should be packed
> + QemuUUID namespace;
> + uint8_t value;
> + uint8_t padding;
> + /* Name */
(42) probably worth mentioning the link
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
around these parts too, not just in patch 8.
> +};
> +
> +
> +#endif /* QEMU_UEFI_VAR_SERVICE_EDK2_H */
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 01/16] hw/uefi: add include/hw/uefi/var-service-api.h Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 02/16] hw/uefi: add include/hw/uefi/var-service-edk2.h Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-17 14:11 ` Laszlo Ersek
2023-11-15 15:12 ` [PATCH 04/16] hw/uefi: add var-service-guid.c Gerd Hoffmann
` (13 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Add state structs and function declarations for the uefi-vars device.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/hw/uefi/var-service.h | 119 ++++++++++++++++++++++++++++++++++
1 file changed, 119 insertions(+)
create mode 100644 include/hw/uefi/var-service.h
diff --git a/include/hw/uefi/var-service.h b/include/hw/uefi/var-service.h
new file mode 100644
index 000000000000..2b8d3052e59f
--- /dev/null
+++ b/include/hw/uefi/var-service.h
@@ -0,0 +1,119 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi-vars device - state struct and function prototypes
+ */
+#ifndef QEMU_UEFI_VAR_SERVICE_H
+#define QEMU_UEFI_VAR_SERVICE_H
+
+#include "qemu/uuid.h"
+#include "qemu/queue.h"
+
+#include "hw/uefi/var-service-edk2.h"
+
+#define MAX_BUFFER_SIZE (64 * 1024)
+
+typedef struct uefi_variable uefi_variable;
+typedef struct uefi_var_policy uefi_var_policy;
+typedef struct uefi_vars_state uefi_vars_state;
+
+struct uefi_variable {
+ QemuUUID guid;
+ uint16_t *name;
+ uint32_t name_size;
+ uint32_t attributes;
+ void *data;
+ uint32_t data_size;
+ QTAILQ_ENTRY(uefi_variable) next;
+};
+
+struct uefi_var_policy {
+ variable_policy_entry *entry;
+ uint32_t entry_size;
+ uint16_t *name;
+ uint32_t name_size;
+ uint32_t hashmarks;
+ QTAILQ_ENTRY(uefi_var_policy) next;
+};
+
+struct uefi_vars_state {
+ MemoryRegion mr;
+ uint16_t sts;
+ uint32_t buf_size;
+ uint32_t buf_addr_lo;
+ uint32_t buf_addr_hi;
+ uint8_t *buffer;
+ QTAILQ_HEAD(, uefi_variable) variables;
+ QTAILQ_HEAD(, uefi_var_policy) var_policies;
+
+ /* boot phases */
+ bool end_of_dxe;
+ bool ready_to_boot;
+ bool exit_boot_service;
+ bool policy_locked;
+
+ /* storage accounting */
+ uint64_t max_storage;
+ uint64_t used_storage;
+
+ char *jsonfile;
+ int jsonfd;
+};
+
+/* vars-service-guid.c */
+extern QemuUUID EfiGlobalVariable;
+extern QemuUUID EfiImageSecurityDatabase;
+extern QemuUUID EfiCustomModeEnable;
+extern QemuUUID EfiSecureBootEnableDisable;
+extern QemuUUID EfiSmmVariableProtocolGuid;
+extern QemuUUID VarCheckPolicyLibMmiHandlerGuid;
+extern QemuUUID EfiEndOfDxeEventGroupGuid;
+extern QemuUUID EfiEventReadyToBootGuid;
+extern QemuUUID EfiEventExitBootServicesGuid;
+
+/* vars-service-core.c */
+extern const VMStateDescription vmstate_uefi_vars;
+size_t uefi_strlen(const uint16_t *str, size_t len);
+gboolean uefi_str_equal(const uint16_t *a, size_t alen,
+ const uint16_t *b, size_t blen);
+char *uefi_ucs2_to_ascii(const uint16_t *ucs2, uint64_t ucs2_size);
+void uefi_trace_variable(const char *action, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size);
+void uefi_trace_status(const char *action, efi_status status);
+void uefi_vars_init(Object *obj, uefi_vars_state *uv);
+void uefi_vars_realize(uefi_vars_state *uv, Error **errp);
+void uefi_vars_hard_reset(uefi_vars_state *uv);
+
+/* vars-service-json.c */
+void uefi_vars_json_init(uefi_vars_state *uv, Error **errp);
+void uefi_vars_json_save(uefi_vars_state *uv);
+void uefi_vars_json_load(uefi_vars_state *uv, Error **errp);
+
+/* vars-service-vars.c */
+extern const VMStateDescription vmstate_uefi_variable;
+uefi_variable *uefi_vars_find_variable(uefi_vars_state *uv, QemuUUID guid,
+ const uint16_t *name,
+ uint64_t name_size);
+void uefi_vars_set_variable(uefi_vars_state *uv, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size,
+ uint32_t attributes,
+ void *data, uint64_t data_size);
+void uefi_vars_clear_volatile(uefi_vars_state *uv);
+void uefi_vars_clear_all(uefi_vars_state *uv);
+void uefi_vars_update_storage(uefi_vars_state *uv);
+uint32_t uefi_vars_mm_vars_proto(uefi_vars_state *uv);
+
+/* vars-service-auth.c */
+void uefi_vars_auth_init(uefi_vars_state *uv);
+
+/* vars-service-policy.c */
+extern const VMStateDescription vmstate_uefi_var_policy;
+efi_status uefi_vars_policy_check(uefi_vars_state *uv,
+ uefi_variable *var,
+ gboolean is_newvar);
+void uefi_vars_policies_clear(uefi_vars_state *uv);
+uefi_var_policy *uefi_vars_add_policy(uefi_vars_state *uv,
+ variable_policy_entry *pe);
+uint32_t uefi_vars_mm_check_policy_proto(uefi_vars_state *uv);
+
+#endif /* QEMU_UEFI_VAR_SERVICE_H */
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
2023-11-15 15:12 ` [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h Gerd Hoffmann
@ 2023-11-17 14:11 ` Laszlo Ersek
2023-11-22 15:12 ` Gerd Hoffmann
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-17 14:11 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau, Daniel P. Berrangé,
graf, Philippe Mathieu-Daudé, Markus Armbruster
On 11/15/23 16:12, Gerd Hoffmann wrote:
> Add state structs and function declarations for the uefi-vars device.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> include/hw/uefi/var-service.h | 119 ++++++++++++++++++++++++++++++++++
> 1 file changed, 119 insertions(+)
> create mode 100644 include/hw/uefi/var-service.h
>
> diff --git a/include/hw/uefi/var-service.h b/include/hw/uefi/var-service.h
> new file mode 100644
> index 000000000000..2b8d3052e59f
> --- /dev/null
> +++ b/include/hw/uefi/var-service.h
> @@ -0,0 +1,119 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi-vars device - state struct and function prototypes
> + */
> +#ifndef QEMU_UEFI_VAR_SERVICE_H
> +#define QEMU_UEFI_VAR_SERVICE_H
> +
> +#include "qemu/uuid.h"
> +#include "qemu/queue.h"
> +
> +#include "hw/uefi/var-service-edk2.h"
> +
> +#define MAX_BUFFER_SIZE (64 * 1024)
> +
> +typedef struct uefi_variable uefi_variable;
> +typedef struct uefi_var_policy uefi_var_policy;
> +typedef struct uefi_vars_state uefi_vars_state;
> +
> +struct uefi_variable {
> + QemuUUID guid;
> + uint16_t *name;
> + uint32_t name_size;
> + uint32_t attributes;
> + void *data;
> + uint32_t data_size;
> + QTAILQ_ENTRY(uefi_variable) next;
> +};
> +
> +struct uefi_var_policy {
> + variable_policy_entry *entry;
> + uint32_t entry_size;
> + uint16_t *name;
> + uint32_t name_size;
> + uint32_t hashmarks;
> + QTAILQ_ENTRY(uefi_var_policy) next;
> +};
- I wonder if the size fields should be size_t. uint32_t is not wrong
either; we'll just have to be careful when doing comparisons etc.
- care to explain (in a comment) hashmarks? I think it's related to the
wildcard policy stuff, but a hint would be appreciated.
> +
> +struct uefi_vars_state {
> + MemoryRegion mr;
> + uint16_t sts;
> + uint32_t buf_size;
> + uint32_t buf_addr_lo;
> + uint32_t buf_addr_hi;
spelling out endianness here would be useful IMO
> + uint8_t *buffer;
> + QTAILQ_HEAD(, uefi_variable) variables;
> + QTAILQ_HEAD(, uefi_var_policy) var_policies;
> +
> + /* boot phases */
> + bool end_of_dxe;
> + bool ready_to_boot;
> + bool exit_boot_service;
There are some variations of the 8 possible that don't make sense. at
the same time, a single enum could be too limiting. depends on what the
code will do with these.
> + bool policy_locked;
> +
> + /* storage accounting */
> + uint64_t max_storage;
> + uint64_t used_storage;
> +
> + char *jsonfile;
> + int jsonfd;
> +};
> +
> +/* vars-service-guid.c */
> +extern QemuUUID EfiGlobalVariable;
> +extern QemuUUID EfiImageSecurityDatabase;
> +extern QemuUUID EfiCustomModeEnable;
> +extern QemuUUID EfiSecureBootEnableDisable;
> +extern QemuUUID EfiSmmVariableProtocolGuid;
> +extern QemuUUID VarCheckPolicyLibMmiHandlerGuid;
> +extern QemuUUID EfiEndOfDxeEventGroupGuid;
> +extern QemuUUID EfiEventReadyToBootGuid;
> +extern QemuUUID EfiEventExitBootServicesGuid;
the spelling of these names appears a bit questionable:
- camelcase is idiomatic in edk2, but (I think?) not in QEMU, for variables
- the "Guid" suffix is inconsistently used / carried over from edk2
> +
> +/* vars-service-core.c */
> +extern const VMStateDescription vmstate_uefi_vars;
> +size_t uefi_strlen(const uint16_t *str, size_t len);
> +gboolean uefi_str_equal(const uint16_t *a, size_t alen,
> + const uint16_t *b, size_t blen);
> +char *uefi_ucs2_to_ascii(const uint16_t *ucs2, uint64_t ucs2_size);
> +void uefi_trace_variable(const char *action, QemuUUID guid,
> + const uint16_t *name, uint64_t name_size);
> +void uefi_trace_status(const char *action, efi_status status);
> +void uefi_vars_init(Object *obj, uefi_vars_state *uv);
> +void uefi_vars_realize(uefi_vars_state *uv, Error **errp);
> +void uefi_vars_hard_reset(uefi_vars_state *uv);
> +
> +/* vars-service-json.c */
> +void uefi_vars_json_init(uefi_vars_state *uv, Error **errp);
> +void uefi_vars_json_save(uefi_vars_state *uv);
> +void uefi_vars_json_load(uefi_vars_state *uv, Error **errp);
> +
> +/* vars-service-vars.c */
> +extern const VMStateDescription vmstate_uefi_variable;
> +uefi_variable *uefi_vars_find_variable(uefi_vars_state *uv, QemuUUID guid,
> + const uint16_t *name,
> + uint64_t name_size);
> +void uefi_vars_set_variable(uefi_vars_state *uv, QemuUUID guid,
> + const uint16_t *name, uint64_t name_size,
> + uint32_t attributes,
> + void *data, uint64_t data_size);
> +void uefi_vars_clear_volatile(uefi_vars_state *uv);
> +void uefi_vars_clear_all(uefi_vars_state *uv);
> +void uefi_vars_update_storage(uefi_vars_state *uv);
> +uint32_t uefi_vars_mm_vars_proto(uefi_vars_state *uv);
> +
> +/* vars-service-auth.c */
> +void uefi_vars_auth_init(uefi_vars_state *uv);
> +
> +/* vars-service-policy.c */
> +extern const VMStateDescription vmstate_uefi_var_policy;
> +efi_status uefi_vars_policy_check(uefi_vars_state *uv,
> + uefi_variable *var,
> + gboolean is_newvar);
> +void uefi_vars_policies_clear(uefi_vars_state *uv);
> +uefi_var_policy *uefi_vars_add_policy(uefi_vars_state *uv,
> + variable_policy_entry *pe);
> +uint32_t uefi_vars_mm_check_policy_proto(uefi_vars_state *uv);
> +
> +#endif /* QEMU_UEFI_VAR_SERVICE_H */
I guess I'll have to see these in use to think anything of them.
(I prefer a more "functional" structuring for a series, where the thing
sort of builds & works from patch#1 onwards, it's only the actual
functionality that is introduced layer by layer. But, that's not an
objection; this patch certainly works as the collection of APIs the rest
is going to implement and call later.)
Again we'll have to keep an eye on the integer types.
with some comments inserted:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h
2023-11-17 14:11 ` Laszlo Ersek
@ 2023-11-22 15:12 ` Gerd Hoffmann
0 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-22 15:12 UTC (permalink / raw)
To: Laszlo Ersek
Cc: qemu-devel, qemu-arm, Eric Blake, Thomas Huth, Michael Roth,
Paolo Bonzini, Peter Maydell, Marc-André Lureau,
Daniel P. Berrangé, graf, Philippe Mathieu-Daudé,
Markus Armbruster
Hi,
> > +struct uefi_var_policy {
> > + variable_policy_entry *entry;
> > + uint32_t entry_size;
> > + uint16_t *name;
> > + uint32_t name_size;
> > + uint32_t hashmarks;
> > + QTAILQ_ENTRY(uefi_var_policy) next;
> > +};
>
> - I wonder if the size fields should be size_t. uint32_t is not wrong
> either; we'll just have to be careful when doing comparisons etc.
I can't store size_t in vmstate ...
> - care to explain (in a comment) hashmarks? I think it's related to the
> wildcard policy stuff, but a hint would be appreciated.
Yes. It's the number of hashmarks, used when sorting entries by
priority. Most specific match, i.e. smallest number of wildcard
characters, goes first.
I'll add a comment.
> > +struct uefi_vars_state {
> > + MemoryRegion mr;
> > + uint16_t sts;
> > + uint32_t buf_size;
> > + uint32_t buf_addr_lo;
> > + uint32_t buf_addr_hi;
>
> spelling out endianness here would be useful IMO
Don't think this is needed, qemu handles this for us. The memory
region is declared to be little endian, qemu will convert reads/writes
to native endian for us, so there are no endian worries for the register
interface (the transfer buffer in guest ram is a different story).
> > + /* boot phases */
> > + bool end_of_dxe;
> > + bool ready_to_boot;
> > + bool exit_boot_service;
>
> There are some variations of the 8 possible that don't make sense. at
> the same time, a single enum could be too limiting. depends on what the
> code will do with these.
end-of-dxe: used by variable policies (they are enforced only after that
event).
ready-to-boot: not used yet (other than setting it when the event arrives).
exit-boot-service: access control (RT vs. BT etc).
> > +/* vars-service-guid.c */
> > +extern QemuUUID EfiGlobalVariable;
> > +extern QemuUUID EfiImageSecurityDatabase;
> > +extern QemuUUID EfiCustomModeEnable;
> > +extern QemuUUID EfiSecureBootEnableDisable;
> > +extern QemuUUID EfiSmmVariableProtocolGuid;
> > +extern QemuUUID VarCheckPolicyLibMmiHandlerGuid;
> > +extern QemuUUID EfiEndOfDxeEventGroupGuid;
> > +extern QemuUUID EfiEventReadyToBootGuid;
> > +extern QemuUUID EfiEventExitBootServicesGuid;
>
> the spelling of these names appears a bit questionable:
>
> - camelcase is idiomatic in edk2, but (I think?) not in QEMU, for variables
>
> - the "Guid" suffix is inconsistently used / carried over from edk2
Yes. It's carried over from edk2.
We don't have to keep the names in qemu, in fact I've renamed the
structs because that would have been too much of a contrast to the
qemu code style, so the edk2 name is only noted in a comment in the
vars-service-edk2.h header file.
For the #defines and GUIDs I've kept the edk2 names as-is, so it's
easier to follow the control flow for people which are familiar with
edk2. We have already have simliar stuff else, for example the struct
names in hw/usb/desc.h follow the usb spec not qemu code style.
take care,
Gerd
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 04/16] hw/uefi: add var-service-guid.c
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (2 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 03/16] hw/uefi: add include/hw/uefi/var-service.h Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-21 13:42 ` Laszlo Ersek
2023-11-15 15:12 ` [PATCH 05/16] hw/uefi: add var-service-core.c Gerd Hoffmann
` (12 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Add variables for a bunch of GUIDs we will need.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-guid.c | 61 ++++++++++++++++++++++++++++++++++++++
1 file changed, 61 insertions(+)
create mode 100644 hw/uefi/var-service-guid.c
diff --git a/hw/uefi/var-service-guid.c b/hw/uefi/var-service-guid.c
new file mode 100644
index 000000000000..afdc15c4e7e6
--- /dev/null
+++ b/hw/uefi/var-service-guid.c
@@ -0,0 +1,61 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - GUIDs
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/dma.h"
+
+#include "hw/uefi/var-service.h"
+
+/* variable namespaces */
+
+QemuUUID EfiGlobalVariable = {
+ .data = UUID_LE(0x8be4df61, 0x93ca, 0x11d2, 0xaa, 0x0d,
+ 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
+};
+
+QemuUUID EfiImageSecurityDatabase = {
+ .data = UUID_LE(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc,
+ 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
+};
+
+QemuUUID EfiCustomModeEnable = {
+ .data = UUID_LE(0xc076ec0c, 0x7028, 0x4399, 0xa0, 0x72,
+ 0x71, 0xee, 0x5c, 0x44, 0x8b, 0x9f)
+};
+
+QemuUUID EfiSecureBootEnableDisable = {
+ .data = UUID_LE(0xf0a30bc7, 0xaf08, 0x4556, 0x99, 0xc4,
+ 0x0, 0x10, 0x9, 0xc9, 0x3a, 0x44)
+};
+
+/* protocols */
+
+QemuUUID EfiSmmVariableProtocolGuid = {
+ .data = UUID_LE(0xed32d533, 0x99e6, 0x4209, 0x9c, 0xc0,
+ 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
+};
+
+QemuUUID VarCheckPolicyLibMmiHandlerGuid = {
+ .data = UUID_LE(0xda1b0d11, 0xd1a7, 0x46c4, 0x9d, 0xc9,
+ 0xf3, 0x71, 0x48, 0x75, 0xc6, 0xeb)
+};
+
+/* events */
+
+QemuUUID EfiEndOfDxeEventGroupGuid = {
+ .data = UUID_LE(0x02CE967A, 0xDD7E, 0x4FFC, 0x9E, 0xE7,
+ 0x81, 0x0C, 0xF0, 0x47, 0x08, 0x80)
+};
+
+QemuUUID EfiEventReadyToBootGuid = {
+ .data = UUID_LE(0x7CE88FB3, 0x4BD7, 0x4679, 0x87, 0xA8,
+ 0xA8, 0xD8, 0xDE, 0xE5, 0x0D, 0x2B)
+};
+
+QemuUUID EfiEventExitBootServicesGuid = {
+ .data = UUID_LE(0x27ABF055, 0xB1B8, 0x4C26, 0x80, 0x48,
+ 0x74, 0x8F, 0x37, 0xBA, 0xA2, 0xDF)
+};
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 04/16] hw/uefi: add var-service-guid.c
2023-11-15 15:12 ` [PATCH 04/16] hw/uefi: add var-service-guid.c Gerd Hoffmann
@ 2023-11-21 13:42 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-21 13:42 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau, Daniel P. Berrangé,
graf, Philippe Mathieu-Daudé, Markus Armbruster
On 11/15/23 16:12, Gerd Hoffmann wrote:
> Add variables for a bunch of GUIDs we will need.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/uefi/var-service-guid.c | 61 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
> create mode 100644 hw/uefi/var-service-guid.c
>
> diff --git a/hw/uefi/var-service-guid.c b/hw/uefi/var-service-guid.c
> new file mode 100644
> index 000000000000..afdc15c4e7e6
> --- /dev/null
> +++ b/hw/uefi/var-service-guid.c
> @@ -0,0 +1,61 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi vars device - GUIDs
> + */
> +
> +#include "qemu/osdep.h"
> +#include "sysemu/dma.h"
> +
> +#include "hw/uefi/var-service.h"
> +
> +/* variable namespaces */
> +
> +QemuUUID EfiGlobalVariable = {
> + .data = UUID_LE(0x8be4df61, 0x93ca, 0x11d2, 0xaa, 0x0d,
> + 0x00, 0xe0, 0x98, 0x03, 0x2b, 0x8c)
> +};
(1) should have asked under patch#3:
can we constify these?
> +
> +QemuUUID EfiImageSecurityDatabase = {
> + .data = UUID_LE(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc,
> + 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
> +};
> +
> +QemuUUID EfiCustomModeEnable = {
> + .data = UUID_LE(0xc076ec0c, 0x7028, 0x4399, 0xa0, 0x72,
> + 0x71, 0xee, 0x5c, 0x44, 0x8b, 0x9f)
> +};
> +
> +QemuUUID EfiSecureBootEnableDisable = {
> + .data = UUID_LE(0xf0a30bc7, 0xaf08, 0x4556, 0x99, 0xc4,
> + 0x0, 0x10, 0x9, 0xc9, 0x3a, 0x44)
> +};
> +
> +/* protocols */
> +
> +QemuUUID EfiSmmVariableProtocolGuid = {
> + .data = UUID_LE(0xed32d533, 0x99e6, 0x4209, 0x9c, 0xc0,
> + 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
> +};
> +
> +QemuUUID VarCheckPolicyLibMmiHandlerGuid = {
> + .data = UUID_LE(0xda1b0d11, 0xd1a7, 0x46c4, 0x9d, 0xc9,
> + 0xf3, 0x71, 0x48, 0x75, 0xc6, 0xeb)
> +};
This isn't really a protocol, but a GUID for "mm_header.guid".
gEfiSmmVariableProtocolGuid is a bit messy in edk2 because (IIUC) it is
used for *three* purposes:
(a) it's an actual SMM protocol GUID (MmVariableServiceInitialize() --
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c),
(b) it's used as "mm_header.guid" (see the MmiHandlerRegister call in
the same location), i.e. basically an SMI "upcall" identifier
(c) it's used as a NULL interface DXE protocol GUID for notification
purposes (VariableNotifySmmReady() --
MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c).
So calling gEfiSmmVariableProtocolGuid a "protocol" in this header file
is relatively justified. But calling "VarCheckPolicyLibMmiHandlerGuid" a
protocol doesn't seem justified, because (I think) it only qualifies for
usage (b).
> +
> +/* events */
More precisely, this would be "event groups"; but peaking ahead to the
next patch, I think all five of these GUIDs (protocols + events) are
just "mm_header.guid" values.
So here's what I propose:
(2) replace
/* protocols */
with
/* mm_header.guid values that the guest DXE/BDS phases use for
* sending requests to management mode
*/
and
(3) replace
/* events */
with
/* mm_header.guid values that the guest DXE/BDS phases use for
* reporting event groups being signaled to management mode
*/
> +
> +QemuUUID EfiEndOfDxeEventGroupGuid = {
> + .data = UUID_LE(0x02CE967A, 0xDD7E, 0x4FFC, 0x9E, 0xE7,
> + 0x81, 0x0C, 0xF0, 0x47, 0x08, 0x80)
> +};
(4) I suggest consistently using either the lowercase hex characters
[a-f] or the uppercase ones [A-F] across all GUID constants in this header.
> +
> +QemuUUID EfiEventReadyToBootGuid = {
> + .data = UUID_LE(0x7CE88FB3, 0x4BD7, 0x4679, 0x87, 0xA8,
> + 0xA8, 0xD8, 0xDE, 0xE5, 0x0D, 0x2B)
> +};
> +
> +QemuUUID EfiEventExitBootServicesGuid = {
> + .data = UUID_LE(0x27ABF055, 0xB1B8, 0x4C26, 0x80, 0x48,
> + 0x74, 0x8F, 0x37, 0xBA, 0xA2, 0xDF)
> +};
I've made an effort to verify the constants themselves; they look good.
Thanks
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 05/16] hw/uefi: add var-service-core.c
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (3 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 04/16] hw/uefi: add var-service-guid.c Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-22 12:25 ` Laszlo Ersek
2023-11-15 15:12 ` [PATCH 06/16] hw/uefi: add var-service-vars.c Gerd Hoffmann
` (11 subsequent siblings)
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This is the core code for guest <-> host communication. This accepts
request messages from the guest, dispatches them to the service called,
and sends back the response message.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-core.c | 350 +++++++++++++++++++++++++++++++++++++
1 file changed, 350 insertions(+)
create mode 100644 hw/uefi/var-service-core.c
diff --git a/hw/uefi/var-service-core.c b/hw/uefi/var-service-core.c
new file mode 100644
index 000000000000..b37f5c403d2f
--- /dev/null
+++ b/hw/uefi/var-service-core.c
@@ -0,0 +1,350 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device
+ */
+#include "qemu/osdep.h"
+#include "sysemu/dma.h"
+#include "migration/vmstate.h"
+
+#include "hw/uefi/var-service.h"
+#include "hw/uefi/var-service-api.h"
+#include "hw/uefi/var-service-edk2.h"
+
+#include "trace/trace-hw_uefi.h"
+
+static int uefi_vars_pre_load(void *opaque)
+{
+ uefi_vars_state *uv = opaque;
+
+ uefi_vars_clear_all(uv);
+ uefi_vars_policies_clear(uv);
+ g_free(uv->buffer);
+ return 0;
+}
+
+static int uefi_vars_post_load(void *opaque, int version_id)
+{
+ uefi_vars_state *uv = opaque;
+
+ uefi_vars_update_storage(uv);
+ uv->buffer = g_malloc(uv->buf_size);
+ return 0;
+}
+
+const VMStateDescription vmstate_uefi_vars = {
+ .name = "uefi-vars",
+ .pre_load = uefi_vars_pre_load,
+ .post_load = uefi_vars_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT16(sts, uefi_vars_state),
+ VMSTATE_UINT32(buf_size, uefi_vars_state),
+ VMSTATE_UINT32(buf_addr_lo, uefi_vars_state),
+ VMSTATE_UINT32(buf_addr_hi, uefi_vars_state),
+ VMSTATE_BOOL(end_of_dxe, uefi_vars_state),
+ VMSTATE_BOOL(ready_to_boot, uefi_vars_state),
+ VMSTATE_BOOL(exit_boot_service, uefi_vars_state),
+ VMSTATE_BOOL(policy_locked, uefi_vars_state),
+ VMSTATE_UINT64(used_storage, uefi_vars_state),
+ VMSTATE_QTAILQ_V(variables, uefi_vars_state, 0,
+ vmstate_uefi_variable, uefi_variable, next),
+ VMSTATE_QTAILQ_V(var_policies, uefi_vars_state, 0,
+ vmstate_uefi_var_policy, uefi_var_policy, next),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+size_t uefi_strlen(const uint16_t *str, size_t len)
+{
+ size_t pos = 0;
+
+ for (;;) {
+ if (pos == len) {
+ return pos;
+ }
+ if (str[pos] == 0) {
+ return pos;
+ }
+ pos++;
+ }
+}
+
+gboolean uefi_str_equal(const uint16_t *a, size_t alen,
+ const uint16_t *b, size_t blen)
+{
+ size_t pos = 0;
+
+ alen = alen / 2;
+ blen = blen / 2;
+ for (;;) {
+ if (pos == alen && pos == blen) {
+ return true;
+ }
+ if (pos == alen && b[pos] == 0) {
+ return true;
+ }
+ if (pos == blen && a[pos] == 0) {
+ return true;
+ }
+ if (pos == alen || pos == blen) {
+ return false;
+ }
+ if (a[pos] == 0 && b[pos] == 0) {
+ return true;
+ }
+ if (a[pos] != b[pos]) {
+ return false;
+ }
+ pos++;
+ }
+}
+
+char *uefi_ucs2_to_ascii(const uint16_t *ucs2, uint64_t ucs2_size)
+{
+ char *str = g_malloc0(ucs2_size / 2 + 1);
+ int i;
+
+ for (i = 0; i * 2 < ucs2_size; i++) {
+ if (ucs2[i] == 0) {
+ break;
+ }
+ if (ucs2[i] < 128) {
+ str[i] = ucs2[i];
+ } else {
+ str[i] = '?';
+ }
+ }
+ str[i] = 0;
+ return str;
+}
+
+void uefi_trace_variable(const char *action, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size)
+{
+ QemuUUID be = qemu_uuid_bswap(guid);
+ char *str_uuid = qemu_uuid_unparse_strdup(&be);
+ char *str_name = uefi_ucs2_to_ascii(name, name_size);
+
+ trace_uefi_variable(action, str_name, name_size, str_uuid);
+
+ g_free(str_name);
+ g_free(str_uuid);
+}
+
+void uefi_trace_status(const char *action, efi_status status)
+{
+ switch (status) {
+ case EFI_SUCCESS:
+ trace_uefi_status(action, "success");
+ break;
+ case EFI_INVALID_PARAMETER:
+ trace_uefi_status(action, "invalid parameter");
+ break;
+ case EFI_UNSUPPORTED:
+ trace_uefi_status(action, "unsupported");
+ break;
+ case EFI_BAD_BUFFER_SIZE:
+ trace_uefi_status(action, "bad buffer size");
+ break;
+ case EFI_BUFFER_TOO_SMALL:
+ trace_uefi_status(action, "buffer too small");
+ break;
+ case EFI_WRITE_PROTECTED:
+ trace_uefi_status(action, "write protected");
+ break;
+ case EFI_OUT_OF_RESOURCES:
+ trace_uefi_status(action, "out of resources");
+ break;
+ case EFI_NOT_FOUND:
+ trace_uefi_status(action, "not found");
+ break;
+ case EFI_ACCESS_DENIED:
+ trace_uefi_status(action, "access denied");
+ break;
+ case EFI_ALREADY_STARTED:
+ trace_uefi_status(action, "already started");
+ break;
+ default:
+ trace_uefi_status(action, "unknown error");
+ break;
+ }
+}
+
+static uint32_t uefi_vars_cmd_mm(uefi_vars_state *uv)
+{
+ hwaddr dma;
+ mm_header *mhdr;
+ uint32_t size, retval;
+
+ dma = uv->buf_addr_lo | ((hwaddr)uv->buf_addr_hi << 32);
+ mhdr = (mm_header *) uv->buffer;
+
+ if (!uv->buffer || uv->buf_size < sizeof(*mhdr)) {
+ return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
+ }
+
+ /* read header */
+ dma_memory_read(&address_space_memory, dma,
+ uv->buffer, sizeof(*mhdr),
+ MEMTXATTRS_UNSPECIFIED);
+
+ size = sizeof(*mhdr) + mhdr->length;
+ if (uv->buf_size < size) {
+ return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
+ }
+
+ /* read buffer (excl header) */
+ dma_memory_read(&address_space_memory, dma + sizeof(*mhdr),
+ uv->buffer + sizeof(*mhdr), mhdr->length,
+ MEMTXATTRS_UNSPECIFIED);
+ memset(uv->buffer + size, 0, uv->buf_size - size);
+
+ /* dispatch */
+ if (qemu_uuid_is_equal(&mhdr->guid, &EfiSmmVariableProtocolGuid)) {
+ retval = uefi_vars_mm_vars_proto(uv);
+
+ } else if (qemu_uuid_is_equal(&mhdr->guid, &VarCheckPolicyLibMmiHandlerGuid)) {
+ retval = uefi_vars_mm_check_policy_proto(uv);
+
+ } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEndOfDxeEventGroupGuid)) {
+ trace_uefi_event("end-of-dxe");
+ uv->end_of_dxe = true;
+ retval = UEFI_VARS_STS_SUCCESS;
+
+ } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEventReadyToBootGuid)) {
+ trace_uefi_event("ready-to-boot");
+ uv->ready_to_boot = true;
+ retval = UEFI_VARS_STS_SUCCESS;
+
+ } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEventExitBootServicesGuid)) {
+ trace_uefi_event("exit-boot-service");
+ uv->exit_boot_service = true;
+ retval = UEFI_VARS_STS_SUCCESS;
+
+ } else {
+ retval = UEFI_VARS_STS_ERR_NOT_SUPPORTED;
+ }
+
+ /* write buffer */
+ dma_memory_write(&address_space_memory, dma,
+ uv->buffer, sizeof(*mhdr) + mhdr->length,
+ MEMTXATTRS_UNSPECIFIED);
+
+ return retval;
+}
+
+static void uefi_vars_soft_reset(uefi_vars_state *uv)
+{
+ g_free(uv->buffer);
+ uv->buffer = NULL;
+ uv->buf_size = 0;
+ uv->buf_addr_lo = 0;
+ uv->buf_addr_hi = 0;
+}
+
+void uefi_vars_hard_reset(uefi_vars_state *uv)
+{
+ trace_uefi_hard_reset();
+ uefi_vars_soft_reset(uv);
+
+ uv->end_of_dxe = false;
+ uv->ready_to_boot = false;
+ uv->exit_boot_service = false;
+ uv->policy_locked = false;
+
+ uefi_vars_clear_volatile(uv);
+ uefi_vars_policies_clear(uv);
+ uefi_vars_auth_init(uv);
+}
+
+static uint32_t uefi_vars_cmd(uefi_vars_state *uv, uint32_t cmd)
+{
+ switch (cmd) {
+ case UEFI_VARS_CMD_RESET:
+ uefi_vars_soft_reset(uv);
+ return UEFI_VARS_STS_SUCCESS;
+ case UEFI_VARS_CMD_MM:
+ return uefi_vars_cmd_mm(uv);
+ default:
+ return UEFI_VARS_STS_ERR_NOT_SUPPORTED;
+ }
+}
+
+static uint64_t uefi_vars_read(void *opaque, hwaddr addr, unsigned size)
+{
+ uefi_vars_state *uv = opaque;
+ uint64_t retval = -1;
+
+ trace_uefi_reg_read(addr, size);
+
+ switch (addr) {
+ case UEFI_VARS_REG_MAGIC:
+ retval = UEFI_VARS_MAGIC_VALUE;
+ break;
+ case UEFI_VARS_REG_CMD_STS:
+ retval = uv->sts;
+ break;
+ case UEFI_VARS_REG_BUFFER_SIZE:
+ retval = uv->buf_size;
+ break;
+ case UEFI_VARS_REG_BUFFER_ADDR_LO:
+ retval = uv->buf_addr_lo;
+ break;
+ case UEFI_VARS_REG_BUFFER_ADDR_HI:
+ retval = uv->buf_addr_hi;
+ break;
+ }
+ return retval;
+}
+
+static void uefi_vars_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
+{
+ uefi_vars_state *uv = opaque;
+
+ trace_uefi_reg_write(addr, val, size);
+
+ switch (addr) {
+ case UEFI_VARS_REG_CMD_STS:
+ uv->sts = uefi_vars_cmd(uv, val);
+ break;
+ case UEFI_VARS_REG_BUFFER_SIZE:
+ if (val > MAX_BUFFER_SIZE) {
+ val = MAX_BUFFER_SIZE;
+ }
+ uv->buf_size = val;
+ g_free(uv->buffer);
+ uv->buffer = g_malloc(uv->buf_size);
+ break;
+ case UEFI_VARS_REG_BUFFER_ADDR_LO:
+ uv->buf_addr_lo = val;
+ break;
+ case UEFI_VARS_REG_BUFFER_ADDR_HI:
+ uv->buf_addr_hi = val;
+ break;
+ }
+}
+
+static const MemoryRegionOps uefi_vars_ops = {
+ .read = uefi_vars_read,
+ .write = uefi_vars_write,
+ .endianness = DEVICE_LITTLE_ENDIAN,
+ .impl = {
+ .min_access_size = 2,
+ .max_access_size = 4,
+ },
+};
+
+void uefi_vars_init(Object *obj, uefi_vars_state *uv)
+{
+ QTAILQ_INIT(&uv->variables);
+ QTAILQ_INIT(&uv->var_policies);
+ uv->jsonfd = -1;
+ memory_region_init_io(&uv->mr, obj, &uefi_vars_ops, uv,
+ "uefi-vars", UEFI_VARS_REGS_SIZE);
+}
+
+void uefi_vars_realize(uefi_vars_state *uv, Error **errp)
+{
+ uefi_vars_json_init(uv, errp);
+ uefi_vars_json_load(uv, errp);
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 05/16] hw/uefi: add var-service-core.c
2023-11-15 15:12 ` [PATCH 05/16] hw/uefi: add var-service-core.c Gerd Hoffmann
@ 2023-11-22 12:25 ` Laszlo Ersek
2023-11-22 16:30 ` Gerd Hoffmann
0 siblings, 1 reply; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-22 12:25 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau, Daniel P. Berrangé,
graf, Philippe Mathieu-Daudé, Markus Armbruster
On 11/15/23 16:12, Gerd Hoffmann wrote:
> This is the core code for guest <-> host communication. This accepts
> request messages from the guest, dispatches them to the service called,
> and sends back the response message.
>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> hw/uefi/var-service-core.c | 350 +++++++++++++++++++++++++++++++++++++
> 1 file changed, 350 insertions(+)
> create mode 100644 hw/uefi/var-service-core.c
If other reviewers don't object, I'd like to request a respin (of the
series) at this point, and I'll stop reviewing this version here:
- This patch is too large for me. It does migration, UCS2 string
utilities, tracing and device code all in one. I think it should be
split in at least three patches (the changes can go in the same "core" C
source file, but in smaller building blocks).
- in general, we should filter out surrogate code points, for any use.
any UCS2 string from the guest that contains a surrogate code point
should be considered invalid, and the request should be rejected based
just on that.
- after splitting, the resultant parts of this patch should be moved
near the end of the series. It references a bunch of helper functions
(which is fine), but for mentally resolving those, in particular for
understanding the life cycles of the various "uefi_vars_state" fields
*here*, I first need to see the internals of the helper functions. If I
jump back and forth between the patches, that's unwieldy for
interrupting the review one day & resuming it another day. The patches
should be included in "topological order" (dependency order) in the
series. The series already follows this idea roughly, AFAICT, but the
placement of this particular patch seems to stick out.
- life cycle comments on the "uefi_vars_state" fields would be
appreciated in the header file, too.
- a tidbit: "uefi_vars_policies_clear" should be spelled
"uefi_var_policies_clear" (i.e., "var" in singular), for consistency
with the field "var_policies" (it's not called "vars_policies").
Thanks,
Laszlo
>
> diff --git a/hw/uefi/var-service-core.c b/hw/uefi/var-service-core.c
> new file mode 100644
> index 000000000000..b37f5c403d2f
> --- /dev/null
> +++ b/hw/uefi/var-service-core.c
> @@ -0,0 +1,350 @@
> +/*
> + * SPDX-License-Identifier: GPL-2.0-or-later
> + *
> + * uefi vars device
> + */
> +#include "qemu/osdep.h"
> +#include "sysemu/dma.h"
> +#include "migration/vmstate.h"
> +
> +#include "hw/uefi/var-service.h"
> +#include "hw/uefi/var-service-api.h"
> +#include "hw/uefi/var-service-edk2.h"
> +
> +#include "trace/trace-hw_uefi.h"
> +
> +static int uefi_vars_pre_load(void *opaque)
> +{
> + uefi_vars_state *uv = opaque;
> +
> + uefi_vars_clear_all(uv);
> + uefi_vars_policies_clear(uv);
> + g_free(uv->buffer);
> + return 0;
> +}
> +
> +static int uefi_vars_post_load(void *opaque, int version_id)
> +{
> + uefi_vars_state *uv = opaque;
> +
> + uefi_vars_update_storage(uv);
> + uv->buffer = g_malloc(uv->buf_size);
> + return 0;
> +}
> +
> +const VMStateDescription vmstate_uefi_vars = {
> + .name = "uefi-vars",
> + .pre_load = uefi_vars_pre_load,
> + .post_load = uefi_vars_post_load,
> + .fields = (VMStateField[]) {
> + VMSTATE_UINT16(sts, uefi_vars_state),
> + VMSTATE_UINT32(buf_size, uefi_vars_state),
> + VMSTATE_UINT32(buf_addr_lo, uefi_vars_state),
> + VMSTATE_UINT32(buf_addr_hi, uefi_vars_state),
> + VMSTATE_BOOL(end_of_dxe, uefi_vars_state),
> + VMSTATE_BOOL(ready_to_boot, uefi_vars_state),
> + VMSTATE_BOOL(exit_boot_service, uefi_vars_state),
> + VMSTATE_BOOL(policy_locked, uefi_vars_state),
> + VMSTATE_UINT64(used_storage, uefi_vars_state),
> + VMSTATE_QTAILQ_V(variables, uefi_vars_state, 0,
> + vmstate_uefi_variable, uefi_variable, next),
> + VMSTATE_QTAILQ_V(var_policies, uefi_vars_state, 0,
> + vmstate_uefi_var_policy, uefi_var_policy, next),
> + VMSTATE_END_OF_LIST()
> + },
> +};
> +
> +size_t uefi_strlen(const uint16_t *str, size_t len)
> +{
> + size_t pos = 0;
> +
> + for (;;) {
> + if (pos == len) {
> + return pos;
> + }
> + if (str[pos] == 0) {
> + return pos;
> + }
> + pos++;
> + }
> +}
> +
> +gboolean uefi_str_equal(const uint16_t *a, size_t alen,
> + const uint16_t *b, size_t blen)
> +{
> + size_t pos = 0;
> +
> + alen = alen / 2;
> + blen = blen / 2;
> + for (;;) {
> + if (pos == alen && pos == blen) {
> + return true;
> + }
> + if (pos == alen && b[pos] == 0) {
> + return true;
> + }
> + if (pos == blen && a[pos] == 0) {
> + return true;
> + }
> + if (pos == alen || pos == blen) {
> + return false;
> + }
> + if (a[pos] == 0 && b[pos] == 0) {
> + return true;
> + }
> + if (a[pos] != b[pos]) {
> + return false;
> + }
> + pos++;
> + }
> +}
> +
> +char *uefi_ucs2_to_ascii(const uint16_t *ucs2, uint64_t ucs2_size)
> +{
> + char *str = g_malloc0(ucs2_size / 2 + 1);
> + int i;
> +
> + for (i = 0; i * 2 < ucs2_size; i++) {
> + if (ucs2[i] == 0) {
> + break;
> + }
> + if (ucs2[i] < 128) {
> + str[i] = ucs2[i];
> + } else {
> + str[i] = '?';
> + }
> + }
> + str[i] = 0;
> + return str;
> +}
> +
> +void uefi_trace_variable(const char *action, QemuUUID guid,
> + const uint16_t *name, uint64_t name_size)
> +{
> + QemuUUID be = qemu_uuid_bswap(guid);
> + char *str_uuid = qemu_uuid_unparse_strdup(&be);
> + char *str_name = uefi_ucs2_to_ascii(name, name_size);
> +
> + trace_uefi_variable(action, str_name, name_size, str_uuid);
> +
> + g_free(str_name);
> + g_free(str_uuid);
> +}
> +
> +void uefi_trace_status(const char *action, efi_status status)
> +{
> + switch (status) {
> + case EFI_SUCCESS:
> + trace_uefi_status(action, "success");
> + break;
> + case EFI_INVALID_PARAMETER:
> + trace_uefi_status(action, "invalid parameter");
> + break;
> + case EFI_UNSUPPORTED:
> + trace_uefi_status(action, "unsupported");
> + break;
> + case EFI_BAD_BUFFER_SIZE:
> + trace_uefi_status(action, "bad buffer size");
> + break;
> + case EFI_BUFFER_TOO_SMALL:
> + trace_uefi_status(action, "buffer too small");
> + break;
> + case EFI_WRITE_PROTECTED:
> + trace_uefi_status(action, "write protected");
> + break;
> + case EFI_OUT_OF_RESOURCES:
> + trace_uefi_status(action, "out of resources");
> + break;
> + case EFI_NOT_FOUND:
> + trace_uefi_status(action, "not found");
> + break;
> + case EFI_ACCESS_DENIED:
> + trace_uefi_status(action, "access denied");
> + break;
> + case EFI_ALREADY_STARTED:
> + trace_uefi_status(action, "already started");
> + break;
> + default:
> + trace_uefi_status(action, "unknown error");
> + break;
> + }
> +}
> +
> +static uint32_t uefi_vars_cmd_mm(uefi_vars_state *uv)
> +{
> + hwaddr dma;
> + mm_header *mhdr;
> + uint32_t size, retval;
> +
> + dma = uv->buf_addr_lo | ((hwaddr)uv->buf_addr_hi << 32);
> + mhdr = (mm_header *) uv->buffer;
> +
> + if (!uv->buffer || uv->buf_size < sizeof(*mhdr)) {
> + return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
> + }
> +
> + /* read header */
> + dma_memory_read(&address_space_memory, dma,
> + uv->buffer, sizeof(*mhdr),
> + MEMTXATTRS_UNSPECIFIED);
> +
> + size = sizeof(*mhdr) + mhdr->length;
> + if (uv->buf_size < size) {
> + return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
> + }
> +
> + /* read buffer (excl header) */
> + dma_memory_read(&address_space_memory, dma + sizeof(*mhdr),
> + uv->buffer + sizeof(*mhdr), mhdr->length,
> + MEMTXATTRS_UNSPECIFIED);
> + memset(uv->buffer + size, 0, uv->buf_size - size);
> +
> + /* dispatch */
> + if (qemu_uuid_is_equal(&mhdr->guid, &EfiSmmVariableProtocolGuid)) {
> + retval = uefi_vars_mm_vars_proto(uv);
> +
> + } else if (qemu_uuid_is_equal(&mhdr->guid, &VarCheckPolicyLibMmiHandlerGuid)) {
> + retval = uefi_vars_mm_check_policy_proto(uv);
> +
> + } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEndOfDxeEventGroupGuid)) {
> + trace_uefi_event("end-of-dxe");
> + uv->end_of_dxe = true;
> + retval = UEFI_VARS_STS_SUCCESS;
> +
> + } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEventReadyToBootGuid)) {
> + trace_uefi_event("ready-to-boot");
> + uv->ready_to_boot = true;
> + retval = UEFI_VARS_STS_SUCCESS;
> +
> + } else if (qemu_uuid_is_equal(&mhdr->guid, &EfiEventExitBootServicesGuid)) {
> + trace_uefi_event("exit-boot-service");
> + uv->exit_boot_service = true;
> + retval = UEFI_VARS_STS_SUCCESS;
> +
> + } else {
> + retval = UEFI_VARS_STS_ERR_NOT_SUPPORTED;
> + }
> +
> + /* write buffer */
> + dma_memory_write(&address_space_memory, dma,
> + uv->buffer, sizeof(*mhdr) + mhdr->length,
> + MEMTXATTRS_UNSPECIFIED);
> +
> + return retval;
> +}
> +
> +static void uefi_vars_soft_reset(uefi_vars_state *uv)
> +{
> + g_free(uv->buffer);
> + uv->buffer = NULL;
> + uv->buf_size = 0;
> + uv->buf_addr_lo = 0;
> + uv->buf_addr_hi = 0;
> +}
> +
> +void uefi_vars_hard_reset(uefi_vars_state *uv)
> +{
> + trace_uefi_hard_reset();
> + uefi_vars_soft_reset(uv);
> +
> + uv->end_of_dxe = false;
> + uv->ready_to_boot = false;
> + uv->exit_boot_service = false;
> + uv->policy_locked = false;
> +
> + uefi_vars_clear_volatile(uv);
> + uefi_vars_policies_clear(uv);
> + uefi_vars_auth_init(uv);
> +}
> +
> +static uint32_t uefi_vars_cmd(uefi_vars_state *uv, uint32_t cmd)
> +{
> + switch (cmd) {
> + case UEFI_VARS_CMD_RESET:
> + uefi_vars_soft_reset(uv);
> + return UEFI_VARS_STS_SUCCESS;
> + case UEFI_VARS_CMD_MM:
> + return uefi_vars_cmd_mm(uv);
> + default:
> + return UEFI_VARS_STS_ERR_NOT_SUPPORTED;
> + }
> +}
> +
> +static uint64_t uefi_vars_read(void *opaque, hwaddr addr, unsigned size)
> +{
> + uefi_vars_state *uv = opaque;
> + uint64_t retval = -1;
> +
> + trace_uefi_reg_read(addr, size);
> +
> + switch (addr) {
> + case UEFI_VARS_REG_MAGIC:
> + retval = UEFI_VARS_MAGIC_VALUE;
> + break;
> + case UEFI_VARS_REG_CMD_STS:
> + retval = uv->sts;
> + break;
> + case UEFI_VARS_REG_BUFFER_SIZE:
> + retval = uv->buf_size;
> + break;
> + case UEFI_VARS_REG_BUFFER_ADDR_LO:
> + retval = uv->buf_addr_lo;
> + break;
> + case UEFI_VARS_REG_BUFFER_ADDR_HI:
> + retval = uv->buf_addr_hi;
> + break;
> + }
> + return retval;
> +}
> +
> +static void uefi_vars_write(void *opaque, hwaddr addr, uint64_t val, unsigned size)
> +{
> + uefi_vars_state *uv = opaque;
> +
> + trace_uefi_reg_write(addr, val, size);
> +
> + switch (addr) {
> + case UEFI_VARS_REG_CMD_STS:
> + uv->sts = uefi_vars_cmd(uv, val);
> + break;
> + case UEFI_VARS_REG_BUFFER_SIZE:
> + if (val > MAX_BUFFER_SIZE) {
> + val = MAX_BUFFER_SIZE;
> + }
> + uv->buf_size = val;
> + g_free(uv->buffer);
> + uv->buffer = g_malloc(uv->buf_size);
> + break;
> + case UEFI_VARS_REG_BUFFER_ADDR_LO:
> + uv->buf_addr_lo = val;
> + break;
> + case UEFI_VARS_REG_BUFFER_ADDR_HI:
> + uv->buf_addr_hi = val;
> + break;
> + }
> +}
> +
> +static const MemoryRegionOps uefi_vars_ops = {
> + .read = uefi_vars_read,
> + .write = uefi_vars_write,
> + .endianness = DEVICE_LITTLE_ENDIAN,
> + .impl = {
> + .min_access_size = 2,
> + .max_access_size = 4,
> + },
> +};
> +
> +void uefi_vars_init(Object *obj, uefi_vars_state *uv)
> +{
> + QTAILQ_INIT(&uv->variables);
> + QTAILQ_INIT(&uv->var_policies);
> + uv->jsonfd = -1;
> + memory_region_init_io(&uv->mr, obj, &uefi_vars_ops, uv,
> + "uefi-vars", UEFI_VARS_REGS_SIZE);
> +}
> +
> +void uefi_vars_realize(uefi_vars_state *uv, Error **errp)
> +{
> + uefi_vars_json_init(uv, errp);
> + uefi_vars_json_load(uv, errp);
> +}
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 05/16] hw/uefi: add var-service-core.c
2023-11-22 12:25 ` Laszlo Ersek
@ 2023-11-22 16:30 ` Gerd Hoffmann
2023-12-08 12:53 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-22 16:30 UTC (permalink / raw)
To: Laszlo Ersek
Cc: qemu-devel, qemu-arm, Eric Blake, Thomas Huth, Michael Roth,
Paolo Bonzini, Peter Maydell, Marc-André Lureau,
Daniel P. Berrangé, graf, Philippe Mathieu-Daudé,
Markus Armbruster
Hi,
> - in general, we should filter out surrogate code points, for any use.
> any UCS2 string from the guest that contains a surrogate code point
> should be considered invalid, and the request should be rejected based
> just on that.
Something like this?
edk2 seems to be inconsistent with strings, sometimes they are expected
to include a terminating '\0' char (most of the time), sometimes not
(in variable policies for example).
gboolean uefi_str_is_valid(const uint16_t *str, size_t len,
gboolean must_be_null_terminated)
{
size_t pos = 0;
for (;;) {
if (pos == len) {
if (must_be_null_terminated) {
return false;
} else {
return true;
}
}
switch (str[pos]) {
case 0:
/* end of string */
return true;
;;
case 0xd800 ... 0xdfff:
/* outlaw surrogates */
return false;
default:
/* char is good, check next */
break;
}
pos++;
}
}
take care,
Gerd
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 05/16] hw/uefi: add var-service-core.c
2023-11-22 16:30 ` Gerd Hoffmann
@ 2023-12-08 12:53 ` Laszlo Ersek
0 siblings, 0 replies; 32+ messages in thread
From: Laszlo Ersek @ 2023-12-08 12:53 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: qemu-devel, qemu-arm, Eric Blake, Thomas Huth, Michael Roth,
Paolo Bonzini, Peter Maydell, Marc-André Lureau,
Daniel P. Berrangé, graf, Philippe Mathieu-Daudé,
Markus Armbruster
On 11/22/23 17:30, Gerd Hoffmann wrote:
> Hi,
>
>> - in general, we should filter out surrogate code points, for any use.
>> any UCS2 string from the guest that contains a surrogate code point
>> should be considered invalid, and the request should be rejected based
>> just on that.
>
> Something like this?
yes please (except I'd recommend s/outlaw/reject/ in the comment)
Thanks
laszlo
>
> edk2 seems to be inconsistent with strings, sometimes they are expected
> to include a terminating '\0' char (most of the time), sometimes not
> (in variable policies for example).
>
> gboolean uefi_str_is_valid(const uint16_t *str, size_t len,
> gboolean must_be_null_terminated)
> {
> size_t pos = 0;
>
> for (;;) {
> if (pos == len) {
> if (must_be_null_terminated) {
> return false;
> } else {
> return true;
> }
> }
> switch (str[pos]) {
> case 0:
> /* end of string */
> return true;
> ;;
> case 0xd800 ... 0xdfff:
> /* outlaw surrogates */
> return false;
> default:
> /* char is good, check next */
> break;
> }
> pos++;
> }
> }
>
> take care,
> Gerd
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 06/16] hw/uefi: add var-service-vars.c
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (4 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 05/16] hw/uefi: add var-service-core.c Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 07/16] hw/uefi: add var-service-auth.c Gerd Hoffmann
` (10 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This is the uefi variable service (EfiSmmVariableProtocol),
providing functions for reading and writing variables.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-vars.c | 602 +++++++++++++++++++++++++++++++++++++
1 file changed, 602 insertions(+)
create mode 100644 hw/uefi/var-service-vars.c
diff --git a/hw/uefi/var-service-vars.c b/hw/uefi/var-service-vars.c
new file mode 100644
index 000000000000..99851a057bb6
--- /dev/null
+++ b/hw/uefi/var-service-vars.c
@@ -0,0 +1,602 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - EfiSmmVariableProtocol implementation
+ */
+#include "qemu/osdep.h"
+#include "sysemu/dma.h"
+#include "migration/vmstate.h"
+
+#include "hw/uefi/var-service.h"
+#include "hw/uefi/var-service-api.h"
+#include "hw/uefi/var-service-edk2.h"
+
+#include "trace/trace-hw_uefi.h"
+
+const VMStateDescription vmstate_uefi_variable = {
+ .name = "uefi-variable",
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT8_ARRAY_V(guid.data, uefi_variable, sizeof(QemuUUID), 0),
+ VMSTATE_UINT32(name_size, uefi_variable),
+ VMSTATE_UINT32(data_size, uefi_variable),
+ VMSTATE_UINT32(attributes, uefi_variable),
+ VMSTATE_VBUFFER_ALLOC_UINT32(name, uefi_variable, 0, NULL, name_size),
+ VMSTATE_VBUFFER_ALLOC_UINT32(data, uefi_variable, 0, NULL, data_size),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+uefi_variable *uefi_vars_find_variable(uefi_vars_state *uv, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size)
+{
+ uefi_variable *var;
+
+ QTAILQ_FOREACH(var, &uv->variables, next) {
+ if (!uefi_str_equal(var->name, var->name_size,
+ name, name_size)) {
+ continue;
+ }
+ if (!qemu_uuid_is_equal(&var->guid, &guid)) {
+ continue;
+ }
+ return var;
+ }
+ return NULL;
+}
+
+static uefi_variable *add_variable(uefi_vars_state *uv, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size,
+ uint32_t attributes)
+{
+ uefi_variable *var;
+
+ var = g_new0(uefi_variable, 1);
+ var->guid = guid;
+ var->name = g_malloc(name_size);
+ memcpy(var->name, name, name_size);
+ var->name_size = name_size;
+ var->attributes = attributes;
+
+ QTAILQ_INSERT_TAIL(&uv->variables, var, next);
+ return var;
+}
+
+static void del_variable(uefi_vars_state *uv, uefi_variable *var)
+{
+ if (!var) {
+ return;
+ }
+
+ QTAILQ_REMOVE(&uv->variables, var, next);
+ g_free(var->data);
+ g_free(var->name);
+ g_free(var);
+}
+
+static size_t variable_size(uefi_variable *var)
+{
+ size_t size;
+
+ size = sizeof(*var);
+ size += var->name_size;
+ size += var->data_size;
+ return size;
+}
+
+void uefi_vars_set_variable(uefi_vars_state *uv, QemuUUID guid,
+ const uint16_t *name, uint64_t name_size,
+ uint32_t attributes,
+ void *data, uint64_t data_size)
+{
+ uefi_variable *old_var, *new_var;
+
+ uefi_trace_variable(__func__, guid, name, name_size);
+
+ old_var = uefi_vars_find_variable(uv, guid, name, name_size);
+ if (old_var) {
+ uv->used_storage -= variable_size(old_var);
+ del_variable(uv, old_var);
+ }
+
+ new_var = add_variable(uv, guid, name, name_size, attributes);
+ new_var->data = g_malloc(data_size);
+ new_var->data_size = data_size;
+ memcpy(new_var->data, data, data_size);
+ uv->used_storage += variable_size(new_var);
+}
+
+void uefi_vars_clear_volatile(uefi_vars_state *uv)
+{
+ uefi_variable *var, *n;
+
+ QTAILQ_FOREACH_SAFE(var, &uv->variables, next, n) {
+ if (var->attributes & EFI_VARIABLE_NON_VOLATILE) {
+ continue;
+ }
+ uv->used_storage -= variable_size(var);
+ del_variable(uv, var);
+ }
+}
+
+void uefi_vars_clear_all(uefi_vars_state *uv)
+{
+ uefi_variable *var, *n;
+
+ QTAILQ_FOREACH_SAFE(var, &uv->variables, next, n) {
+ del_variable(uv, var);
+ }
+ uv->used_storage = 0;
+}
+
+void uefi_vars_update_storage(uefi_vars_state *uv)
+{
+ uefi_variable *var;
+
+ uv->used_storage = 0;
+ QTAILQ_FOREACH(var, &uv->variables, next) {
+ uv->used_storage += variable_size(var);
+ }
+}
+
+static efi_status check_secure_boot(uefi_vars_state *uv, uefi_variable *var)
+{
+ static const uint16_t pk[] = { 'P', 'K', 0 };
+ static const uint16_t kek[] = { 'K', 'E', 'K', 0 };
+ static const uint16_t db[] = { 'd', 'b', 0 };
+ static const uint16_t dbx[] = { 'd', 'b', 'x', 0 };
+
+ /* TODO (reject for now) */
+ if (qemu_uuid_is_equal(&var->guid, &EfiGlobalVariable) &&
+ uefi_str_equal(var->name, var->name_size, pk, sizeof(pk))) {
+ return EFI_WRITE_PROTECTED;
+ }
+ if (qemu_uuid_is_equal(&var->guid, &EfiGlobalVariable) &&
+ uefi_str_equal(var->name, var->name_size, kek, sizeof(kek))) {
+ return EFI_WRITE_PROTECTED;
+ }
+
+ if (qemu_uuid_is_equal(&var->guid, &EfiImageSecurityDatabase) &&
+ uefi_str_equal(var->name, var->name_size, db, sizeof(db))) {
+ return EFI_WRITE_PROTECTED;
+ }
+ if (qemu_uuid_is_equal(&var->guid, &EfiImageSecurityDatabase) &&
+ uefi_str_equal(var->name, var->name_size, dbx, sizeof(dbx))) {
+ return EFI_WRITE_PROTECTED;
+ }
+
+ return EFI_SUCCESS;
+}
+
+static gboolean check_access(uefi_vars_state *uv, uefi_variable *var)
+{
+ if (!uv->exit_boot_service) {
+ if (!(var->attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)) {
+ return false;
+ }
+ } else {
+ if (!(var->attributes & EFI_VARIABLE_RUNTIME_ACCESS)) {
+ return false;
+ }
+ }
+ return true;
+}
+
+static efi_status check_update(uefi_vars_state *uv, uefi_variable *old_var,
+ uefi_variable *new_var)
+{
+ efi_status status;
+
+ if (old_var) {
+ if (!check_access(uv, old_var)) {
+ return EFI_ACCESS_DENIED;
+ }
+ if (old_var->attributes &
+ (EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS |
+ EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)) {
+ /* TODO (reject for now) */
+ return EFI_WRITE_PROTECTED;
+ }
+ }
+
+ if (new_var) {
+ if (!check_access(uv, new_var)) {
+ return EFI_ACCESS_DENIED;
+ }
+ }
+
+ if (old_var && new_var) {
+ if (old_var->attributes != new_var->attributes) {
+ return EFI_INVALID_PARAMETER;
+ }
+ }
+
+ if (new_var) {
+ /* create + update */
+ status = uefi_vars_policy_check(uv, new_var, old_var == NULL);
+ } else if (old_var) {
+ /* delete */
+ status = uefi_vars_policy_check(uv, old_var, false);
+ }
+ if (status != EFI_SUCCESS) {
+ return status;
+ }
+
+ status = check_secure_boot(uv, new_var ?: old_var);
+ if (status != EFI_SUCCESS) {
+ return status;
+ }
+
+ return EFI_SUCCESS;
+}
+
+static size_t uefi_vars_mm_error(mm_header *mhdr, mm_variable *mvar,
+ uint64_t status)
+{
+ mvar->status = status;
+ return sizeof(*mvar);
+}
+
+static size_t uefi_vars_mm_get_variable(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_variable_access *va = func;
+ uint16_t *name;
+ void *data;
+ uefi_variable *var;
+ size_t length;
+
+ length = sizeof(*mvar) + sizeof(*va);
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ if (va->name_size > uv->max_storage ||
+ va->data_size > uv->max_storage) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_OUT_OF_RESOURCES);
+ }
+
+ name = func + sizeof(*va);
+ length += va->name_size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ uefi_trace_variable(__func__, va->guid, name, va->name_size);
+
+ var = uefi_vars_find_variable(uv, va->guid, name, va->name_size);
+ if (!var) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_NOT_FOUND);
+ }
+
+ /* check permissions etc. */
+ if (!check_access(uv, var)) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_ACCESS_DENIED);
+ }
+
+ data = func + sizeof(*va) + va->name_size;
+ length += var->data_size;
+ if (uv->buf_size < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ va->attributes = var->attributes;
+ va->data_size = var->data_size;
+ memcpy(data, var->data, var->data_size);
+ mvar->status = EFI_SUCCESS;
+ return length;
+}
+
+static size_t
+uefi_vars_mm_get_next_variable(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_next_variable *nv = func;
+ uefi_variable *var;
+ uint16_t *name;
+ size_t length;
+
+ length = sizeof(*mvar) + sizeof(*nv);
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ if (nv->name_size > uv->max_storage) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_OUT_OF_RESOURCES);
+ }
+
+ name = func + sizeof(*nv);
+ length += nv->name_size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ if (uefi_strlen(name, nv->name_size) == 0) {
+ /* empty string -> first */
+ var = QTAILQ_FIRST(&uv->variables);
+ if (!var) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_NOT_FOUND);
+ }
+ } else {
+ var = uefi_vars_find_variable(uv, nv->guid, name, nv->name_size);
+ if (!var) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_INVALID_PARAMETER);
+ }
+ do {
+ var = QTAILQ_NEXT(var, next);
+ } while (var && !check_access(uv, var));
+ if (!var) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_NOT_FOUND);
+ }
+ }
+
+ length = sizeof(*mvar) + sizeof(*nv) + var->name_size;
+ if (uv->buf_size < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ nv->guid = var->guid;
+ nv->name_size = var->name_size;
+ memcpy(name, var->name, var->name_size);
+ mvar->status = EFI_SUCCESS;
+ return length;
+}
+
+static size_t uefi_vars_mm_set_variable(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_variable_access *va = func;
+ uint32_t attributes = 0;
+ uint16_t *name;
+ void *data;
+ uefi_variable *old_var, *new_var;
+ size_t length, new_storage;
+ efi_status status;
+
+ length = sizeof(*mvar) + sizeof(*va);
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ if (va->name_size > uv->max_storage ||
+ va->data_size > uv->max_storage) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_OUT_OF_RESOURCES);
+ }
+
+ name = func + sizeof(*va);
+ length += va->name_size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ data = func + sizeof(*va) + va->name_size;
+ length += va->data_size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ g_assert(va->name_size < G_MAXUINT32);
+ g_assert(va->data_size < G_MAXUINT32);
+
+ uefi_trace_variable(__func__, va->guid, name, va->name_size);
+
+ old_var = uefi_vars_find_variable(uv, va->guid, name, va->name_size);
+ if (va->data_size) {
+ new_var = add_variable(uv, va->guid, name, va->name_size,
+ va->attributes);
+ new_var->data = g_malloc(va->data_size);
+ memcpy(new_var->data, data, va->data_size);
+ new_var->data_size = va->data_size;
+ } else {
+ new_var = NULL;
+ }
+
+ if (!old_var && !new_var) {
+ /* delete non-existing variable -> nothing to do */
+ mvar->status = EFI_SUCCESS;
+ return sizeof(*mvar);
+ }
+
+ /* check permissions etc. */
+ status = check_update(uv, old_var, new_var);
+ if (status != EFI_SUCCESS) {
+ mvar->status = status;
+ goto rollback;
+ }
+
+ /* check storage space */
+ new_storage = uv->used_storage;
+ if (old_var) {
+ new_storage -= variable_size(old_var);
+ }
+ if (new_var) {
+ new_storage += variable_size(new_var);
+ }
+ if (new_storage > uv->max_storage) {
+ mvar->status = EFI_OUT_OF_RESOURCES;
+ goto rollback;
+ }
+
+ attributes = new_var
+ ? new_var->attributes
+ : old_var->attributes;
+
+ /* all good, commit */
+ del_variable(uv, old_var);
+ uv->used_storage = new_storage;
+
+ if (attributes & EFI_VARIABLE_NON_VOLATILE) {
+ uefi_vars_json_save(uv);
+ }
+
+ mvar->status = EFI_SUCCESS;
+ return sizeof(*mvar);
+
+rollback:
+ del_variable(uv, new_var);
+ return sizeof(*mvar);
+}
+
+static size_t uefi_vars_mm_variable_info(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_variable_info *vi = func;
+ size_t length;
+
+ length = sizeof(*mvar) + sizeof(*vi);
+ if (uv->buf_size < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ vi->max_storage_size = uv->max_storage;
+ vi->free_storage_size = uv->max_storage - uv->used_storage;
+ vi->max_variable_size = uv->max_storage >> 2;
+ vi->attributes = 0;
+
+ mvar->status = EFI_SUCCESS;
+ return length;
+}
+
+static size_t
+uefi_vars_mm_get_payload_size(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_get_payload_size *ps = func;
+ size_t length;
+
+ length = sizeof(*mvar) + sizeof(*ps);
+ if (uv->buf_size < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ ps->payload_size = uv->buf_size;
+ mvar->status = EFI_SUCCESS;
+ return length;
+}
+
+static size_t
+uefi_vars_mm_lock_variable(uefi_vars_state *uv, mm_header *mhdr,
+ mm_variable *mvar, void *func)
+{
+ mm_lock_variable *lv = func;
+ variable_policy_entry *pe;
+ uint16_t *name, *dest;
+ size_t length;
+
+ length = sizeof(*mvar) + sizeof(*lv);
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ name = func + sizeof(*lv);
+ length += lv->name_size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_error(mhdr, mvar, EFI_BAD_BUFFER_SIZE);
+ }
+
+ uefi_trace_variable(__func__, lv->guid, name, lv->name_size);
+
+ pe = g_malloc0(sizeof(*pe) + lv->name_size);
+ pe->version = VARIABLE_POLICY_ENTRY_REVISION;
+ pe->size = sizeof(*pe) + lv->name_size;
+ pe->offset_to_name = sizeof(*pe);
+ pe->namespace = lv->guid;
+ pe->min_size = 0;
+ pe->max_size = UINT32_MAX;
+ pe->attributes_must_have = 0;
+ pe->attributes_cant_have = 0;
+ pe->lock_policy_type = VARIABLE_POLICY_TYPE_LOCK_NOW;
+
+ dest = (void *)pe + pe->offset_to_name;
+ memcpy(dest, name, lv->name_size);
+
+ uefi_vars_add_policy(uv, pe);
+ g_free(pe);
+
+ mvar->status = EFI_SUCCESS;
+ return length;
+}
+
+uint32_t uefi_vars_mm_vars_proto(uefi_vars_state *uv)
+{
+ static const char *fnames[] = {
+ "zero",
+ "get-variable",
+ "get-next-variable-name",
+ "set-variable",
+ "query-variable-info",
+ "ready-to-boot",
+ "exit-boot-service",
+ "get-statistics",
+ "lock-variable",
+ "var-check-prop-set",
+ "var-check-prop-get",
+ "get-payload-size",
+ "init-runtime-cache-contect",
+ "sync-runtime-cache",
+ "get-runtime-cache-info",
+ };
+ const char *fname;
+ size_t length;
+
+ mm_header *mhdr = (mm_header *) uv->buffer;
+ mm_variable *mvar = (mm_variable *) (uv->buffer + sizeof(*mhdr));
+ void *func = (uv->buffer + sizeof(*mhdr) + sizeof(*mvar));
+
+ if (mhdr->length < sizeof(*mvar)) {
+ return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
+ }
+
+ fname = mvar->function < ARRAY_SIZE(fnames)
+ ? fnames[mvar->function]
+ : "unknown";
+ trace_uefi_vars_proto_cmd(fname);
+
+ switch (mvar->function) {
+ case SMM_VARIABLE_FUNCTION_GET_VARIABLE:
+ length = uefi_vars_mm_get_variable(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME:
+ length = uefi_vars_mm_get_next_variable(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_SET_VARIABLE:
+ length = uefi_vars_mm_set_variable(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO:
+ length = uefi_vars_mm_variable_info(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_LOCK_VARIABLE:
+ length = uefi_vars_mm_lock_variable(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE:
+ length = uefi_vars_mm_get_payload_size(uv, mhdr, mvar, func);
+ break;
+
+ case SMM_VARIABLE_FUNCTION_READY_TO_BOOT:
+ trace_uefi_event("ready-to-boot");
+ uv->ready_to_boot = true;
+ length = 0;
+ break;
+
+ case SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE:
+ trace_uefi_event("exit-boot-service");
+ uv->exit_boot_service = true;
+ length = 0;
+ break;
+
+ default:
+ length = uefi_vars_mm_error(mhdr, mvar, EFI_UNSUPPORTED);
+ break;
+ }
+
+ if (mhdr->length < length) {
+ mvar->status = EFI_BUFFER_TOO_SMALL;
+ }
+
+ uefi_trace_status(__func__, mvar->status);
+ return UEFI_VARS_STS_SUCCESS;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 07/16] hw/uefi: add var-service-auth.c
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (5 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 06/16] hw/uefi: add var-service-vars.c Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 08/16] hw/uefi: add var-service-policy.c Gerd Hoffmann
` (9 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This implements authenticated variable handling (AuthVariableLib in edk2).
For now this implements the bare minimum to make secure boot work,
by initializing the 'SecureBoot' variable.
Support for authenticated variable updates is not implemented yet, for
now they are read-only so the guest can neither provision secure boot
keys nor update the 'dbx' database.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-auth.c | 91 ++++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
create mode 100644 hw/uefi/var-service-auth.c
diff --git a/hw/uefi/var-service-auth.c b/hw/uefi/var-service-auth.c
new file mode 100644
index 000000000000..e7cff65275c2
--- /dev/null
+++ b/hw/uefi/var-service-auth.c
@@ -0,0 +1,91 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - AuthVariableLib
+ */
+
+#include "qemu/osdep.h"
+#include "sysemu/dma.h"
+
+#include "hw/uefi/var-service.h"
+
+static const uint16_t name_pk[] = { 'P', 'K',
+ 0 };
+static const uint16_t name_setup_mode[] = { 'S', 'e', 't', 'u', 'p',
+ 'M', 'o', 'd', 'e',
+ 0 };
+static const uint16_t name_sb[] = { 'S', 'e', 'c', 'u', 'r', 'e',
+ 'B', 'o', 'o', 't',
+ 0 };
+static const uint16_t name_sb_enable[] = { 'S', 'e', 'c', 'u', 'r', 'e',
+ 'B', 'o', 'o', 't',
+ 'E', 'n', 'a', 'b', 'l', 'e',
+ 0 };
+static const uint16_t name_custom_mode[] = { 'C', 'u', 's', 't', 'o', 'm',
+ 'M', 'o', 'd', 'e',
+ 0 };
+
+/* AuthVariableLibInitialize */
+void uefi_vars_auth_init(uefi_vars_state *uv)
+{
+ uefi_variable *pk_var, *sbe_var;;
+ uint8_t platform_mode, sb, sbe, custom_mode;
+
+ /* SetupMode */
+ pk_var = uefi_vars_find_variable(uv, EfiGlobalVariable,
+ name_pk, sizeof(name_pk));
+ if (!pk_var) {
+ platform_mode = SETUP_MODE;
+ } else {
+ platform_mode = USER_MODE;
+ }
+ uefi_vars_set_variable(uv, EfiGlobalVariable,
+ name_setup_mode, sizeof(name_setup_mode),
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ &platform_mode, sizeof(platform_mode));
+
+ /* TODO: SignatureSupport */
+
+ /* SecureBootEnable */
+ sbe = SECURE_BOOT_DISABLE;
+ sbe_var = uefi_vars_find_variable(uv, EfiSecureBootEnableDisable,
+ name_sb_enable, sizeof(name_sb_enable));
+ if (sbe_var) {
+ if (platform_mode == USER_MODE) {
+ sbe = ((uint8_t*)sbe_var->data)[0];
+ }
+ } else if (platform_mode == USER_MODE) {
+ sbe = SECURE_BOOT_ENABLE;
+ uefi_vars_set_variable(uv, EfiSecureBootEnableDisable,
+ name_sb_enable, sizeof(name_sb_enable),
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS,
+ &sbe, sizeof(sbe));
+ }
+
+ /* SecureBoot */
+ if ((sbe == SECURE_BOOT_ENABLE) && (platform_mode == USER_MODE)) {
+ sb = SECURE_BOOT_MODE_ENABLE;
+ } else {
+ sb = SECURE_BOOT_MODE_DISABLE;
+ }
+ uefi_vars_set_variable(uv, EfiGlobalVariable,
+ name_sb, sizeof(name_sb),
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS,
+ &sb, sizeof(sb));
+
+ /* CustomMode */
+ custom_mode = STANDARD_SECURE_BOOT_MODE;
+ uefi_vars_set_variable(uv, EfiCustomModeEnable,
+ name_custom_mode, sizeof(name_custom_mode),
+ EFI_VARIABLE_NON_VOLATILE |
+ EFI_VARIABLE_BOOTSERVICE_ACCESS,
+ &custom_mode, sizeof(custom_mode));
+
+ /* TODO: certdb */
+ /* TODO: certdbv */
+ /* TODO: VendorKeysNv */
+ /* TODO: VendorKeys */
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 08/16] hw/uefi: add var-service-policy.c
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (6 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 07/16] hw/uefi: add var-service-auth.c Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 09/16] hw/uefi: add support for storing persistent variables on disk Gerd Hoffmann
` (8 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Implement variable policies (Edk2VariablePolicyProtocol).
This protocol allows to define restrictions for variables.
It also allows to lock down variables (disallow write access).
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-policy.c | 390 +++++++++++++++++++++++++++++++++++
1 file changed, 390 insertions(+)
create mode 100644 hw/uefi/var-service-policy.c
diff --git a/hw/uefi/var-service-policy.c b/hw/uefi/var-service-policy.c
new file mode 100644
index 000000000000..f44edb358c8f
--- /dev/null
+++ b/hw/uefi/var-service-policy.c
@@ -0,0 +1,390 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - VarCheckPolicyLibMmiHandler implementation
+ *
+ * variable policy specs:
+ * https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md
+ */
+#include "qemu/osdep.h"
+#include "sysemu/dma.h"
+#include "migration/vmstate.h"
+
+#include "hw/uefi/var-service.h"
+#include "hw/uefi/var-service-api.h"
+#include "hw/uefi/var-service-edk2.h"
+
+#include "trace/trace-hw_uefi.h"
+
+static void calc_policy(uefi_var_policy *pol);
+
+static int uefi_var_policy_post_load(void *opaque, int version_id)
+{
+ uefi_var_policy *pol = opaque;
+
+ calc_policy(pol);
+ return 0;
+}
+
+const VMStateDescription vmstate_uefi_var_policy = {
+ .name = "uefi-var-policy",
+ .post_load = uefi_var_policy_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_UINT32(entry_size, uefi_var_policy),
+ VMSTATE_VBUFFER_ALLOC_UINT32(entry, uefi_var_policy,
+ 0, NULL, entry_size),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+static void print_policy_entry(variable_policy_entry *pe)
+{
+ uint16_t *name = (void *)pe + pe->offset_to_name;
+
+ fprintf(stderr, "%s:\n", __func__);
+
+ fprintf(stderr, " name ´");
+ while (*name) {
+ fprintf(stderr, "%c", *name);
+ name++;
+ }
+ fprintf(stderr, "', version=%d.%d, size=%d\n",
+ pe->version >> 16, pe->version & 0xffff, pe->size);
+
+ if (pe->min_size) {
+ fprintf(stderr, " size min=%d\n", pe->min_size);
+ }
+ if (pe->max_size != UINT32_MAX) {
+ fprintf(stderr, " size max=%u\n", pe->max_size);
+ }
+ if (pe->attributes_must_have) {
+ fprintf(stderr, " attr must=0x%x\n", pe->attributes_must_have);
+ }
+ if (pe->attributes_cant_have) {
+ fprintf(stderr, " attr cant=0x%x\n", pe->attributes_cant_have);
+ }
+ if (pe->lock_policy_type) {
+ fprintf(stderr, " lock policy type %d\n", pe->lock_policy_type);
+ }
+}
+
+static gboolean wildcard_strcmp(uefi_var_policy *pol,
+ uefi_variable *var)
+{
+ size_t pos = 0;
+ size_t plen = pol->name_size / 2;
+ size_t vlen = var->name_size / 2;
+
+ if (plen == 0) {
+ return true;
+ }
+
+ for (;;) {
+ if (pos == plen && pos == vlen) {
+ return true;
+ }
+ if (pos == plen || pos == vlen) {
+ return false;
+ }
+ if (pol->name[pos] == 0 && var->name[pos] == 0) {
+ return true;
+ }
+
+ if (pol->name[pos] == '#') {
+ if (!isxdigit(var->name[pos])) {
+ return false;
+ }
+ } else {
+ if (pol->name[pos] != var->name[pos]) {
+ return false;
+ }
+ }
+
+ pos++;
+ }
+}
+
+static uefi_var_policy *find_policy(uefi_vars_state *uv, QemuUUID guid,
+ uint16_t *name, uint64_t name_size)
+{
+ uefi_var_policy *pol;
+
+ QTAILQ_FOREACH(pol, &uv->var_policies, next) {
+ if (!qemu_uuid_is_equal(&pol->entry->namespace, &guid)) {
+ continue;
+ }
+ if (!uefi_str_equal(pol->name, pol->name_size,
+ name, name_size)) {
+ continue;
+ }
+ return pol;
+ }
+ return NULL;
+}
+
+static uefi_var_policy *wildcard_find_policy(uefi_vars_state *uv,
+ uefi_variable *var)
+{
+ uefi_var_policy *pol;
+
+ QTAILQ_FOREACH(pol, &uv->var_policies, next) {
+ if (!qemu_uuid_is_equal(&pol->entry->namespace, &var->guid)) {
+ continue;
+ }
+ if (!wildcard_strcmp(pol, var)) {
+ continue;
+ }
+ return pol;
+ }
+ return NULL;
+}
+
+static void calc_policy(uefi_var_policy *pol)
+{
+ variable_policy_entry *pe = pol->entry;
+ unsigned int i;
+
+ pol->name = (void *)pol->entry + pe->offset_to_name;
+ pol->name_size = pe->size - pe->offset_to_name;
+
+ for (i = 0; i < pol->name_size / 2; i++) {
+ if (pol->name[i] == '#') {
+ pol->hashmarks++;
+ }
+ }
+}
+
+uefi_var_policy *uefi_vars_add_policy(uefi_vars_state *uv,
+ variable_policy_entry *pe)
+{
+ uefi_var_policy *pol, *p;
+
+ pol = g_new0(uefi_var_policy, 1);
+ pol->entry = g_malloc(pe->size);
+ memcpy(pol->entry, pe, pe->size);
+ pol->entry_size = pe->size;
+
+ calc_policy(pol);
+
+ /* keep list sorted by priority, add to tail of priority group */
+ QTAILQ_FOREACH(p, &uv->var_policies, next) {
+ if ((p->hashmarks > pol->hashmarks) ||
+ (!p->name_size && pol->name_size)) {
+ QTAILQ_INSERT_BEFORE(p, pol, next);
+ return pol;
+ }
+ }
+
+ QTAILQ_INSERT_TAIL(&uv->var_policies, pol, next);
+ return pol;
+}
+
+efi_status uefi_vars_policy_check(uefi_vars_state *uv,
+ uefi_variable *var,
+ gboolean is_newvar)
+{
+ uefi_var_policy *pol;
+ variable_policy_entry *pe;
+ variable_lock_on_var_state *lvarstate;
+ uint16_t *lvarname;
+ size_t lvarnamesize;
+ uefi_variable *lvar;
+
+ if (!uv->end_of_dxe) {
+ return EFI_SUCCESS;
+ }
+
+ pol = wildcard_find_policy(uv, var);
+ if (!pol) {
+ return EFI_SUCCESS;
+ }
+ pe = pol->entry;
+
+ uefi_trace_variable(__func__, var->guid, var->name, var->name_size);
+ print_policy_entry(pe);
+
+ if ((var->attributes & pe->attributes_must_have) != pe->attributes_must_have) {
+ trace_uefi_vars_policy_deny("must-have-attr");
+ return EFI_INVALID_PARAMETER;
+ }
+ if ((var->attributes & pe->attributes_cant_have) != 0) {
+ trace_uefi_vars_policy_deny("cant-have-attr");
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (var->data_size < pe->min_size) {
+ trace_uefi_vars_policy_deny("min-size");
+ return EFI_INVALID_PARAMETER;
+ }
+ if (var->data_size > pe->max_size) {
+ trace_uefi_vars_policy_deny("max-size");
+ return EFI_INVALID_PARAMETER;
+ }
+
+ switch (pe->lock_policy_type) {
+ case VARIABLE_POLICY_TYPE_NO_LOCK:
+ break;
+
+ case VARIABLE_POLICY_TYPE_LOCK_NOW:
+ trace_uefi_vars_policy_deny("lock-now");
+ return EFI_WRITE_PROTECTED;
+
+ case VARIABLE_POLICY_TYPE_LOCK_ON_CREATE:
+ if (!is_newvar) {
+ trace_uefi_vars_policy_deny("lock-on-create");
+ return EFI_WRITE_PROTECTED;
+ }
+ break;
+
+ case VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE:
+ lvarstate = (void *)pol->entry + sizeof(*pe);
+ lvarname = (void *)pol->entry + sizeof(*pe) + sizeof(*lvarstate);
+ lvarnamesize = pe->offset_to_name - sizeof(*pe) - sizeof(*lvarstate);
+
+ uefi_trace_variable(__func__, lvarstate->namespace,
+ lvarname, lvarnamesize);
+ lvar = uefi_vars_find_variable(uv, lvarstate->namespace,
+ lvarname, lvarnamesize);
+ if (lvar && lvar->data_size == 1) {
+ uint8_t *value = lvar->data;
+ if (lvarstate->value == *value) {
+ return EFI_WRITE_PROTECTED;
+ }
+ }
+ break;
+ }
+
+ return EFI_SUCCESS;
+}
+
+void uefi_vars_policies_clear(uefi_vars_state *uv)
+{
+ uefi_var_policy *pol;
+
+ while (!QTAILQ_EMPTY(&uv->var_policies)) {
+ pol = QTAILQ_FIRST(&uv->var_policies);
+ QTAILQ_REMOVE(&uv->var_policies, pol, next);
+ g_free(pol->entry);
+ g_free(pol);
+ }
+}
+
+static size_t uefi_vars_mm_policy_error(mm_header *mhdr,
+ mm_check_policy *mchk,
+ uint64_t status)
+{
+ mchk->result = status;
+ return sizeof(*mchk);
+}
+
+static uint32_t uefi_vars_mm_check_policy_is_enabled(uefi_vars_state *uv,
+ mm_header *mhdr,
+ mm_check_policy *mchk,
+ void *func)
+{
+ mm_check_policy_is_enabled *mpar = func;
+ size_t length;
+
+ length = sizeof(*mchk) + sizeof(*mpar);
+ if (mhdr->length < length) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+
+ mpar->state = TRUE;
+ mchk->result = EFI_SUCCESS;
+ return sizeof(*mchk);
+}
+
+static uint32_t uefi_vars_mm_check_policy_register(uefi_vars_state *uv,
+ mm_header *mhdr,
+ mm_check_policy *mchk,
+ void *func)
+{
+ variable_policy_entry *pe = func;
+ uefi_var_policy *pol;
+ size_t length;
+
+ length = sizeof(*mchk) + pe->size;
+ if (mhdr->length < length) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+ if (pe->size < sizeof(*pe)) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+ if (pe->offset_to_name < sizeof(*pe)) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+
+ if (pe->lock_policy_type == VARIABLE_POLICY_TYPE_LOCK_ON_VAR_STATE &&
+ pe->offset_to_name < sizeof(*pe) + sizeof(variable_lock_on_var_state)) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+
+ /* check space for minimum string length */
+ if (pe->size < (size_t)pe->offset_to_name) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_BAD_BUFFER_SIZE);
+ }
+
+ pol = find_policy(uv, pe->namespace,
+ (void *)pe + pe->offset_to_name,
+ pe->size - pe->offset_to_name);
+ if (pol) {
+ return uefi_vars_mm_policy_error(mhdr, mchk, EFI_ALREADY_STARTED);
+ }
+
+ uefi_vars_add_policy(uv, pe);
+
+ mchk->result = EFI_SUCCESS;
+ return sizeof(*mchk);
+}
+
+uint32_t uefi_vars_mm_check_policy_proto(uefi_vars_state *uv)
+{
+ static const char *fnames[] = {
+ "zero",
+ "disable",
+ "is-enabled",
+ "register",
+ "dump",
+ "lock",
+ };
+ const char *fname;
+ mm_header *mhdr = (mm_header *) uv->buffer;
+ mm_check_policy *mchk = (mm_check_policy *) (uv->buffer + sizeof(*mhdr));
+ void *func = (uv->buffer + sizeof(*mhdr) + sizeof(*mchk));
+
+ if (mhdr->length < sizeof(*mchk)) {
+ return UEFI_VARS_STS_ERR_BAD_BUFFER_SIZE;
+ }
+
+ fname = mchk->command < ARRAY_SIZE(fnames)
+ ? fnames[mchk->command]
+ : "unknown";
+ trace_uefi_vars_policy_cmd(fname);
+
+ switch (mchk->command) {
+ case VAR_CHECK_POLICY_COMMAND_DISABLE:
+ mchk->result = EFI_UNSUPPORTED;
+ break;
+ case VAR_CHECK_POLICY_COMMAND_IS_ENABLED:
+ uefi_vars_mm_check_policy_is_enabled(uv, mhdr, mchk, func);
+ break;
+ case VAR_CHECK_POLICY_COMMAND_REGISTER:
+ if (uv->policy_locked) {
+ mchk->result = EFI_WRITE_PROTECTED;
+ } else {
+ uefi_vars_mm_check_policy_register(uv, mhdr, mchk, func);
+ }
+ break;
+ case VAR_CHECK_POLICY_COMMAND_LOCK:
+ uv->policy_locked = true;
+ mchk->result = EFI_SUCCESS;
+ break;
+ default:
+ mchk->result = EFI_UNSUPPORTED;
+ break;
+ }
+
+ uefi_trace_status(__func__, mchk->result);
+ return UEFI_VARS_STS_SUCCESS;
+}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 09/16] hw/uefi: add support for storing persistent variables on disk
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (7 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 08/16] hw/uefi: add var-service-policy.c Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 10/16] hw/uefi: add trace-events Gerd Hoffmann
` (7 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Define qapi schema for the uefi variable store state.
Use it and the generated visitor helper functions to
store persistent variables in JSON format on disk.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-json.c | 194 +++++++++++++++++++++++++++++++++++++
qapi/meson.build | 1 +
qapi/qapi-schema.json | 1 +
qapi/uefi.json | 40 ++++++++
4 files changed, 236 insertions(+)
create mode 100644 hw/uefi/var-service-json.c
create mode 100644 qapi/uefi.json
diff --git a/hw/uefi/var-service-json.c b/hw/uefi/var-service-json.c
new file mode 100644
index 000000000000..d8d74945bbf1
--- /dev/null
+++ b/hw/uefi/var-service-json.c
@@ -0,0 +1,194 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - serialize non-volatile varstore from/to json,
+ * using qapi
+ *
+ * tools which can read/write these json files:
+ * - https://gitlab.com/kraxel/virt-firmware
+ * - https://github.com/awslabs/python-uefivars
+ */
+#include "qemu/osdep.h"
+#include "qemu/cutils.h"
+#include "sysemu/dma.h"
+
+#include "hw/uefi/var-service.h"
+
+#include "qapi/dealloc-visitor.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
+#include "qapi/qmp/qobject.h"
+#include "qapi/qmp/qjson.h"
+#include "qapi/qapi-types-uefi.h"
+#include "qapi/qapi-visit-uefi.h"
+
+static UefiVarStore *uefi_vars_to_qapi(uefi_vars_state *uv)
+{
+ static const char hex[] = {
+ '0', '1', '2', '3', '4', '5', '6', '7',
+ '8', '9', 'a', 'b', 'c', 'd', 'e', 'f',
+ };
+ UefiVarStore *vs;
+ UefiVariableList **tail;
+ UefiVariable *v;
+ QemuUUID be;
+ uefi_variable *var;
+ uint8_t *data;
+ unsigned int i;
+
+ vs = g_new0(UefiVarStore, 1);
+ vs->version = 2;
+ tail = &vs->variables;
+
+ QTAILQ_FOREACH(var, &uv->variables, next) {
+ if (!(var->attributes & EFI_VARIABLE_NON_VOLATILE)) {
+ continue;
+ }
+
+ v = g_new0(UefiVariable, 1);
+ be = qemu_uuid_bswap(var->guid);
+ v->guid = qemu_uuid_unparse_strdup(&be);
+ v->name = uefi_ucs2_to_ascii(var->name, var->name_size);
+ v->attr = var->attributes;
+
+ v->data = g_malloc(var->data_size * 2 + 1);
+ data = var->data;
+ for (i = 0; i < var->data_size * 2;) {
+ v->data[i++] = hex[*data >> 4];
+ v->data[i++] = hex[*data & 15];
+ data++;
+ }
+ v->data[i++] = 0;
+
+ QAPI_LIST_APPEND(tail, v);
+ }
+ return vs;
+}
+
+static unsigned parse_hexchar(char c)
+{
+ switch (c) {
+ case '0' ... '9': return c - '0';
+ case 'a' ... 'f': return c - 'a' + 0xa;
+ case 'A' ... 'F': return c - 'A' + 0xA;
+ default: return 0;
+ }
+}
+
+static void uefi_vars_from_qapi(uefi_vars_state *uv, UefiVarStore *vs)
+{
+ UefiVariableList *item;
+ UefiVariable *v;
+ QemuUUID be;
+ uefi_variable *var;
+ uint8_t *data;
+ size_t i, len;
+
+ for (item = vs->variables; item != NULL; item = item->next) {
+ v = item->value;
+
+ var = g_new0(uefi_variable, 1);
+ var->attributes = v->attr;
+ qemu_uuid_parse(v->guid, &be);
+ var->guid = qemu_uuid_bswap(be);
+
+ len = strlen(v->name);
+ var->name_size = len * 2 + 2;
+ var->name = g_malloc(var->name_size);
+ for (i = 0; i <= len; i++) {
+ var->name[i] = v->name[i];
+ }
+
+ len = strlen(v->data);
+ var->data_size = len / 2;
+ var->data = data = g_malloc(var->data_size);
+ for (i = 0; i < len; i += 2) {
+ *(data++) =
+ parse_hexchar(v->data[i]) << 4 |
+ parse_hexchar(v->data[i + 1]);
+ }
+
+ QTAILQ_INSERT_TAIL(&uv->variables, var, next);
+ }
+}
+
+static GString *uefi_vars_to_json(uefi_vars_state *uv)
+{
+ UefiVarStore *vs = uefi_vars_to_qapi(uv);
+ QObject *qobj = NULL;
+ Visitor *v;
+ GString *gstr;
+
+ v = qobject_output_visitor_new(&qobj);
+ if (visit_type_UefiVarStore(v, NULL, &vs, NULL)) {
+ visit_complete(v, &qobj);
+ }
+ visit_free(v);
+ qapi_free_UefiVarStore(vs);
+
+ gstr = qobject_to_json_pretty(qobj, true);
+ qobject_unref(qobj);
+
+ return gstr;
+}
+
+void uefi_vars_json_init(uefi_vars_state *uv, Error **errp)
+{
+ if (uv->jsonfile) {
+ uv->jsonfd = qemu_create(uv->jsonfile, O_RDWR, 0666, errp);
+ }
+}
+
+void uefi_vars_json_save(uefi_vars_state *uv)
+{
+ GString *gstr;
+
+ if (uv->jsonfd == -1) {
+ return;
+ }
+
+ gstr = uefi_vars_to_json(uv);
+
+ lseek(uv->jsonfd, 0, SEEK_SET);
+ write(uv->jsonfd, gstr->str, gstr->len);
+ ftruncate(uv->jsonfd, gstr->len);
+ fsync(uv->jsonfd);
+
+ g_string_free(gstr, true);
+}
+
+void uefi_vars_json_load(uefi_vars_state *uv, Error **errp)
+{
+ UefiVarStore *vs;
+ QObject *qobj;
+ Visitor *v;
+ char *str;
+ size_t len;
+
+ if (uv->jsonfd == -1) {
+ return;
+ }
+
+ len = lseek(uv->jsonfd, 0, SEEK_END);
+ if (len == 0) {
+ return;
+ }
+
+ str = g_malloc(len + 1);
+ lseek(uv->jsonfd, 0, SEEK_SET);
+ read(uv->jsonfd, str, len);
+ str[len] = 0;
+
+ qobj = qobject_from_json(str, errp);
+ v = qobject_input_visitor_new(qobj);
+ visit_type_UefiVarStore(v, NULL, &vs, errp);
+ visit_free(v);
+
+ if (!(*errp)) {
+ uefi_vars_from_qapi(uv, vs);
+ }
+
+ qapi_free_UefiVarStore(vs);
+ qobject_unref(qobj);
+ g_free(str);
+}
diff --git a/qapi/meson.build b/qapi/meson.build
index f81a37565ca7..0cd1b1e0b2d1 100644
--- a/qapi/meson.build
+++ b/qapi/meson.build
@@ -64,6 +64,7 @@ if have_system
'rdma',
'rocker',
'tpm',
+ 'uefi',
]
endif
if have_system or have_tools
diff --git a/qapi/qapi-schema.json b/qapi/qapi-schema.json
index c01ec335e680..d169b4660b20 100644
--- a/qapi/qapi-schema.json
+++ b/qapi/qapi-schema.json
@@ -80,3 +80,4 @@
{ 'include': 'virtio.json' }
{ 'include': 'cryptodev.json' }
{ 'include': 'cxl.json' }
+{ 'include': 'uefi.json' }
diff --git a/qapi/uefi.json b/qapi/uefi.json
new file mode 100644
index 000000000000..152c4d404824
--- /dev/null
+++ b/qapi/uefi.json
@@ -0,0 +1,40 @@
+# -*- Mode: Python -*-
+# vim: filetype=python
+#
+
+##
+# @UefiVariable:
+#
+# UEFI Variable
+#
+# @guid: variable namespace guid
+#
+# @name: variable name (utf-8)
+#
+# @attr: variable attributes
+#
+# @data: variable content (base64)
+#
+# Since: 9.0
+##
+{ 'struct' : 'UefiVariable',
+ 'data' : { 'guid' : 'str',
+ 'name' : 'str',
+ 'attr' : 'int',
+ 'data' : 'str',
+ '*time' : 'str'}}
+
+##
+# @UefiVarStore:
+#
+# UEFI Variable Store
+#
+# @version: 2
+#
+# @variables: list of uefi variables
+#
+# Since: 9.0
+##
+{ 'struct' : 'UefiVarStore',
+ 'data' : { 'version' : 'int',
+ 'variables' : [ 'UefiVariable' ] }}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 10/16] hw/uefi: add trace-events
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (8 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 09/16] hw/uefi: add support for storing persistent variables on disk Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 11/16] hw/uefi: add to Kconfig Gerd Hoffmann
` (6 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/trace-events | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
create mode 100644 hw/uefi/trace-events
diff --git a/hw/uefi/trace-events b/hw/uefi/trace-events
new file mode 100644
index 000000000000..baeda81bbe12
--- /dev/null
+++ b/hw/uefi/trace-events
@@ -0,0 +1,16 @@
+# device
+uefi_reg_read(uint64_t addr, unsigned size) "addr 0x%lx, size %d"
+uefi_reg_write(uint64_t addr, uint64_t val, unsigned size) "addr 0x%lx, val 0x%lx, size %d"
+uefi_hard_reset(void) ""
+
+# generic uefi
+uefi_variable(const char *context, const char *name, uint64_t size, const char *uuid) "context %s, name %s, size %ld, uuid %s"
+uefi_status(const char *context, const char *name) "context %s, status %s"
+uefi_event(const char *name) "event %s"
+
+# variable protocol
+uefi_vars_proto_cmd(const char *cmd) "cmd %s"
+
+# variable policy protocol
+uefi_vars_policy_cmd(const char *cmd) "cmd %s"
+uefi_vars_policy_deny(const char *reason) "reason %s"
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 11/16] hw/uefi: add to Kconfig
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (9 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 10/16] hw/uefi: add trace-events Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 12/16] hw/uefi: add to meson Gerd Hoffmann
` (5 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/Kconfig | 1 +
hw/uefi/Kconfig | 3 +++
2 files changed, 4 insertions(+)
create mode 100644 hw/uefi/Kconfig
diff --git a/hw/Kconfig b/hw/Kconfig
index 9ca7b38c31f1..af41bd4e0b40 100644
--- a/hw/Kconfig
+++ b/hw/Kconfig
@@ -38,6 +38,7 @@ source smbios/Kconfig
source ssi/Kconfig
source timer/Kconfig
source tpm/Kconfig
+source uefi/Kconfig
source ufs/Kconfig
source usb/Kconfig
source virtio/Kconfig
diff --git a/hw/uefi/Kconfig b/hw/uefi/Kconfig
new file mode 100644
index 000000000000..ca6c2bc46a96
--- /dev/null
+++ b/hw/uefi/Kconfig
@@ -0,0 +1,3 @@
+config UEFI_VARS
+ bool
+ default y if X86_64 || AARCH64
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 12/16] hw/uefi: add to meson
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (10 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 11/16] hw/uefi: add to Kconfig Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 13/16] hw/uefi: add uefi-vars-sysbus device Gerd Hoffmann
` (4 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Wire up uefi-vars in the build system.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/meson.build | 1 +
hw/uefi/meson.build | 12 ++++++++++++
meson.build | 1 +
3 files changed, 14 insertions(+)
create mode 100644 hw/uefi/meson.build
diff --git a/hw/meson.build b/hw/meson.build
index f01fac4617c9..f8a7e4a2d8db 100644
--- a/hw/meson.build
+++ b/hw/meson.build
@@ -37,6 +37,7 @@ subdir('smbios')
subdir('ssi')
subdir('timer')
subdir('tpm')
+subdir('uefi')
subdir('ufs')
subdir('usb')
subdir('vfio')
diff --git a/hw/uefi/meson.build b/hw/uefi/meson.build
new file mode 100644
index 000000000000..b620297320d0
--- /dev/null
+++ b/hw/uefi/meson.build
@@ -0,0 +1,12 @@
+uefi_vars_ss = ss.source_set()
+uefi_vars_ss.add(when: 'CONFIG_UEFI_VARS',
+ if_true: files('var-service-core.c',
+ 'var-service-json.c',
+ 'var-service-vars.c',
+ 'var-service-auth.c',
+ 'var-service-guid.c',
+ 'var-service-policy.c'))
+
+modules += { 'hw-uefi' : {
+ 'vars' : uefi_vars_ss,
+}}
diff --git a/meson.build b/meson.build
index dcef8b1e7911..05a60631a81f 100644
--- a/meson.build
+++ b/meson.build
@@ -3290,6 +3290,7 @@ if have_system
'hw/ssi',
'hw/timer',
'hw/tpm',
+ 'hw/uefi',
'hw/ufs',
'hw/usb',
'hw/vfio',
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 13/16] hw/uefi: add uefi-vars-sysbus device
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (11 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 12/16] hw/uefi: add to meson Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 14/16] hw/uefi: add uefi-vars-isa device Gerd Hoffmann
` (3 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This adds sysbus bindings for the variable service.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-sysbus.c | 87 ++++++++++++++++++++++++++++++++++++
hw/uefi/meson.build | 3 +-
2 files changed, 89 insertions(+), 1 deletion(-)
create mode 100644 hw/uefi/var-service-sysbus.c
diff --git a/hw/uefi/var-service-sysbus.c b/hw/uefi/var-service-sysbus.c
new file mode 100644
index 000000000000..2b393fc768a9
--- /dev/null
+++ b/hw/uefi/var-service-sysbus.c
@@ -0,0 +1,87 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - sysbus variant.
+ */
+#include "qemu/osdep.h"
+#include "migration/vmstate.h"
+
+#include "hw/qdev-properties.h"
+#include "hw/sysbus.h"
+
+#include "hw/uefi/var-service.h"
+#include "hw/uefi/var-service-api.h"
+
+#define TYPE_UEFI_VARS_SYSBUS "uefi-vars-sysbus"
+OBJECT_DECLARE_SIMPLE_TYPE(uefi_vars_sysbus_state, UEFI_VARS_SYSBUS)
+
+struct uefi_vars_sysbus_state {
+ SysBusDevice parent_obj;
+ struct uefi_vars_state state;
+};
+
+static const VMStateDescription vmstate_uefi_vars_sysbus = {
+ .name = "uefi-vars-sysbus",
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(state, uefi_vars_sysbus_state, 0,
+ vmstate_uefi_vars, uefi_vars_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static Property uefi_vars_sysbus_properties[] = {
+ DEFINE_PROP_SIZE("size", uefi_vars_sysbus_state, state.max_storage,
+ 256 * 1024),
+ DEFINE_PROP_STRING("jsonfile", uefi_vars_sysbus_state, state.jsonfile),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void uefi_vars_sysbus_init(Object *obj)
+{
+ uefi_vars_sysbus_state *uv = UEFI_VARS_SYSBUS(obj);
+
+ uefi_vars_init(obj, &uv->state);
+}
+
+static void uefi_vars_sysbus_reset(DeviceState *dev)
+{
+ uefi_vars_sysbus_state *uv = UEFI_VARS_SYSBUS(dev);
+
+ uefi_vars_hard_reset(&uv->state);
+}
+
+static void uefi_vars_sysbus_realize(DeviceState *dev, Error **errp)
+{
+ uefi_vars_sysbus_state *uv = UEFI_VARS_SYSBUS(dev);
+ SysBusDevice *sysbus = SYS_BUS_DEVICE(dev);
+
+ sysbus_init_mmio(sysbus, &uv->state.mr);
+ uefi_vars_realize(&uv->state, errp);
+}
+
+static void uefi_vars_sysbus_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = uefi_vars_sysbus_realize;
+ dc->reset = uefi_vars_sysbus_reset;
+ dc->vmsd = &vmstate_uefi_vars_sysbus;
+ device_class_set_props(dc, uefi_vars_sysbus_properties);
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo uefi_vars_sysbus_info = {
+ .name = TYPE_UEFI_VARS_SYSBUS,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(uefi_vars_sysbus_state),
+ .instance_init = uefi_vars_sysbus_init,
+ .class_init = uefi_vars_sysbus_class_init,
+};
+module_obj(TYPE_UEFI_VARS_SYSBUS);
+
+static void uefi_vars_sysbus_register_types(void)
+{
+ type_register_static(&uefi_vars_sysbus_info);
+}
+
+type_init(uefi_vars_sysbus_register_types)
diff --git a/hw/uefi/meson.build b/hw/uefi/meson.build
index b620297320d0..dc363d67cccc 100644
--- a/hw/uefi/meson.build
+++ b/hw/uefi/meson.build
@@ -5,7 +5,8 @@ uefi_vars_ss.add(when: 'CONFIG_UEFI_VARS',
'var-service-vars.c',
'var-service-auth.c',
'var-service-guid.c',
- 'var-service-policy.c'))
+ 'var-service-policy.c',
+ 'var-service-sysbus.c'))
modules += { 'hw-uefi' : {
'vars' : uefi_vars_ss,
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 14/16] hw/uefi: add uefi-vars-isa device
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (12 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 13/16] hw/uefi: add uefi-vars-sysbus device Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 15/16] hw/arm: add uefi variable support to virt machine type Gerd Hoffmann
` (2 subsequent siblings)
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
This adds isa bindings for the variable service.
Usage: qemu-system-x86_64 -device uefi-vars-isa,jsonfile=/path/to/uefivars.json
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
hw/uefi/var-service-isa.c | 88 +++++++++++++++++++++++++++++++++++++++
hw/uefi/Kconfig | 6 +++
hw/uefi/meson.build | 5 +++
3 files changed, 99 insertions(+)
create mode 100644 hw/uefi/var-service-isa.c
diff --git a/hw/uefi/var-service-isa.c b/hw/uefi/var-service-isa.c
new file mode 100644
index 000000000000..bdb270c2a643
--- /dev/null
+++ b/hw/uefi/var-service-isa.c
@@ -0,0 +1,88 @@
+/*
+ * SPDX-License-Identifier: GPL-2.0-or-later
+ *
+ * uefi vars device - ISA variant for x64.
+ */
+#include "qemu/osdep.h"
+#include "migration/vmstate.h"
+
+#include "hw/isa/isa.h"
+#include "hw/qdev-properties.h"
+
+#include "hw/uefi/var-service.h"
+#include "hw/uefi/var-service-api.h"
+
+#define TYPE_UEFI_VARS_ISA "uefi-vars-isa"
+OBJECT_DECLARE_SIMPLE_TYPE(uefi_vars_isa_state, UEFI_VARS_ISA)
+
+struct uefi_vars_isa_state {
+ ISADevice parent_obj;
+ struct uefi_vars_state state;
+};
+
+static const VMStateDescription vmstate_uefi_vars_isa = {
+ .name = "uefi-vars-isa",
+ .fields = (VMStateField[]) {
+ VMSTATE_STRUCT(state, uefi_vars_isa_state, 0,
+ vmstate_uefi_vars, uefi_vars_state),
+ VMSTATE_END_OF_LIST()
+ }
+};
+
+static Property uefi_vars_isa_properties[] = {
+ DEFINE_PROP_SIZE("size", uefi_vars_isa_state, state.max_storage,
+ 256 * 1024),
+ DEFINE_PROP_STRING("jsonfile", uefi_vars_isa_state, state.jsonfile),
+ DEFINE_PROP_END_OF_LIST(),
+};
+
+static void uefi_vars_isa_init(Object *obj)
+{
+ uefi_vars_isa_state *uv = UEFI_VARS_ISA(obj);
+
+ uefi_vars_init(obj, &uv->state);
+}
+
+static void uefi_vars_isa_reset(DeviceState *dev)
+{
+ uefi_vars_isa_state *uv = UEFI_VARS_ISA(dev);
+
+ uefi_vars_hard_reset(&uv->state);
+}
+
+static void uefi_vars_isa_realize(DeviceState *dev, Error **errp)
+{
+ uefi_vars_isa_state *uv = UEFI_VARS_ISA(dev);
+ ISADevice *isa = ISA_DEVICE(dev);
+
+ isa_register_ioport(isa, &uv->state.mr, UEFI_VARS_IO_BASE);
+ uefi_vars_realize(&uv->state, errp);
+}
+
+static void uefi_vars_isa_class_init(ObjectClass *klass, void *data)
+{
+ DeviceClass *dc = DEVICE_CLASS(klass);
+
+ dc->realize = uefi_vars_isa_realize;
+ dc->reset = uefi_vars_isa_reset;
+ dc->vmsd = &vmstate_uefi_vars_isa;
+ device_class_set_props(dc, uefi_vars_isa_properties);
+ set_bit(DEVICE_CATEGORY_MISC, dc->categories);
+}
+
+static const TypeInfo uefi_vars_isa_info = {
+ .name = TYPE_UEFI_VARS_ISA,
+ .parent = TYPE_ISA_DEVICE,
+ .instance_size = sizeof(uefi_vars_isa_state),
+ .instance_init = uefi_vars_isa_init,
+ .class_init = uefi_vars_isa_class_init,
+};
+module_obj(TYPE_UEFI_VARS_ISA);
+module_dep("hw-uefi-vars");
+
+static void uefi_vars_isa_register_types(void)
+{
+ type_register_static(&uefi_vars_isa_info);
+}
+
+type_init(uefi_vars_isa_register_types)
diff --git a/hw/uefi/Kconfig b/hw/uefi/Kconfig
index ca6c2bc46a96..feb9f6de5e30 100644
--- a/hw/uefi/Kconfig
+++ b/hw/uefi/Kconfig
@@ -1,3 +1,9 @@
config UEFI_VARS
bool
default y if X86_64 || AARCH64
+
+config UEFI_VARS_ISA
+ bool
+ default y
+ depends on UEFI_VARS
+ depends on ISA_BUS
diff --git a/hw/uefi/meson.build b/hw/uefi/meson.build
index dc363d67cccc..959d2a630bbf 100644
--- a/hw/uefi/meson.build
+++ b/hw/uefi/meson.build
@@ -8,6 +8,11 @@ uefi_vars_ss.add(when: 'CONFIG_UEFI_VARS',
'var-service-policy.c',
'var-service-sysbus.c'))
+uefi_vars_isa_ss = ss.source_set()
+uefi_vars_isa_ss.add(when: 'CONFIG_UEFI_VARS_ISA',
+ if_true: files('var-service-isa.c'))
+
modules += { 'hw-uefi' : {
'vars' : uefi_vars_ss,
+ 'vars-isa' : uefi_vars_isa_ss,
}}
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 15/16] hw/arm: add uefi variable support to virt machine type
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (13 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 14/16] hw/uefi: add uefi-vars-isa device Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:12 ` [PATCH 16/16] docs: add uefi variable service documentation and TODO list Gerd Hoffmann
2023-11-20 11:53 ` [PATCH 00/16] hw/uefi: add uefi variable service Alexander Graf
16 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Add -machine virt,x-uefi-vars={on,off} property. Default is off.
When enabled wire up the uefi-vars-sysbus device.
TODO: wire up jsonfile property.
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
include/hw/arm/virt.h | 2 ++
hw/arm/virt.c | 41 +++++++++++++++++++++++++++++++++++++++++
2 files changed, 43 insertions(+)
diff --git a/include/hw/arm/virt.h b/include/hw/arm/virt.h
index f69239850e61..3dd655b880a9 100644
--- a/include/hw/arm/virt.h
+++ b/include/hw/arm/virt.h
@@ -76,6 +76,7 @@ enum {
VIRT_ACPI_GED,
VIRT_NVDIMM_ACPI,
VIRT_PVTIME,
+ VIRT_UEFI_VARS,
VIRT_LOWMEMMAP_LAST,
};
@@ -150,6 +151,7 @@ struct VirtMachineState {
bool ras;
bool mte;
bool dtb_randomness;
+ bool uefi_vars;
OnOffAuto acpi;
VirtGICType gic_version;
VirtIOMMUType iommu;
diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 529f1c089c08..49f692fda7cf 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -65,6 +65,7 @@
#include "hw/intc/arm_gicv3_common.h"
#include "hw/intc/arm_gicv3_its_common.h"
#include "hw/irq.h"
+#include "hw/uefi/var-service-api.h"
#include "kvm_arm.h"
#include "hw/firmware/smbios.h"
#include "qapi/visitor.h"
@@ -155,6 +156,7 @@ static const MemMapEntry base_memmap[] = {
[VIRT_NVDIMM_ACPI] = { 0x09090000, NVDIMM_ACPI_IO_LEN},
[VIRT_PVTIME] = { 0x090a0000, 0x00010000 },
[VIRT_SECURE_GPIO] = { 0x090b0000, 0x00001000 },
+ [VIRT_UEFI_VARS] = { 0x090c0000, 0x00000010 },
[VIRT_MMIO] = { 0x0a000000, 0x00000200 },
/* ...repeating for a total of NUM_VIRTIO_TRANSPORTS, each of that size */
[VIRT_PLATFORM_BUS] = { 0x0c000000, 0x02000000 },
@@ -1296,6 +1298,24 @@ static FWCfgState *create_fw_cfg(const VirtMachineState *vms, AddressSpace *as)
return fw_cfg;
}
+static void create_uefi_vars(const VirtMachineState *vms)
+{
+ hwaddr base = vms->memmap[VIRT_UEFI_VARS].base;
+ hwaddr size = vms->memmap[VIRT_UEFI_VARS].size;
+ MachineState *ms = MACHINE(vms);
+ char *nodename;
+
+ sysbus_create_simple("uefi-vars-sysbus", base, NULL);
+
+ nodename = g_strdup_printf("/%s@%" PRIx64, UEFI_VARS_FDT_NODE, base);
+ qemu_fdt_add_subnode(ms->fdt, nodename);
+ qemu_fdt_setprop_string(ms->fdt, nodename,
+ "compatible", UEFI_VARS_FDT_COMPAT);
+ qemu_fdt_setprop_sized_cells(ms->fdt, nodename, "reg",
+ 2, base, 2, size);
+ g_free(nodename);
+}
+
static void create_pcie_irq_map(const MachineState *ms,
uint32_t gic_phandle,
int first_irq, const char *nodename)
@@ -2306,6 +2326,10 @@ static void machvirt_init(MachineState *machine)
vms->fw_cfg = create_fw_cfg(vms, &address_space_memory);
rom_set_fw(vms->fw_cfg);
+ if (vms->uefi_vars) {
+ create_uefi_vars(vms);
+ }
+
create_platform_bus(vms);
if (machine->nvdimms_state->is_enabled) {
@@ -2502,6 +2526,20 @@ static void virt_set_oem_table_id(Object *obj, const char *value,
strncpy(vms->oem_table_id, value, 8);
}
+static bool virt_get_uefi_vars(Object *obj, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ return vms->uefi_vars;
+}
+
+static void virt_set_uefi_vars(Object *obj, bool value, Error **errp)
+{
+ VirtMachineState *vms = VIRT_MACHINE(obj);
+
+ vms->uefi_vars = value;
+}
+
bool virt_is_acpi_enabled(VirtMachineState *vms)
{
@@ -3092,6 +3130,9 @@ static void virt_machine_class_init(ObjectClass *oc, void *data)
"in ACPI table header."
"The string may be up to 8 bytes in size");
+ object_class_property_add_bool(oc, "x-uefi-vars",
+ virt_get_uefi_vars,
+ virt_set_uefi_vars);
}
static void virt_instance_init(Object *obj)
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* [PATCH 16/16] docs: add uefi variable service documentation and TODO list.
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (14 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 15/16] hw/arm: add uefi variable support to virt machine type Gerd Hoffmann
@ 2023-11-15 15:12 ` Gerd Hoffmann
2023-11-15 15:56 ` Eric Blake
2023-11-20 11:53 ` [PATCH 00/16] hw/uefi: add uefi variable service Alexander Graf
16 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-15 15:12 UTC (permalink / raw)
To: qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster, Gerd Hoffmann
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
docs/devel/index-internals.rst | 1 +
docs/devel/uefi-vars.rst | 66 ++++++++++++++++++++++++++++++++++
hw/uefi/TODO.md | 17 +++++++++
3 files changed, 84 insertions(+)
create mode 100644 docs/devel/uefi-vars.rst
create mode 100644 hw/uefi/TODO.md
diff --git a/docs/devel/index-internals.rst b/docs/devel/index-internals.rst
index 6f81df92bcab..eee676704cfa 100644
--- a/docs/devel/index-internals.rst
+++ b/docs/devel/index-internals.rst
@@ -17,6 +17,7 @@ Details about QEMU's various subsystems including how to add features to them.
s390-cpu-topology
s390-dasd-ipl
tracing
+ uefi-vars
vfio-migration
writing-monitor-commands
virtio-backends
diff --git a/docs/devel/uefi-vars.rst b/docs/devel/uefi-vars.rst
new file mode 100644
index 000000000000..8da69f3545af
--- /dev/null
+++ b/docs/devel/uefi-vars.rst
@@ -0,0 +1,66 @@
+==============
+UEFI variables
+==============
+
+Guest UEFI variable management
+==============================
+
+Traditional approach for UEFI Variable storage in qemu guests is to
+work as close as possible to physical hardware. That means provide
+pflash as storage and leave the management of variables and flash to
+the guest.
+
+Secure boot support comes with the requirement that the UEFI variable
+storage must be protected against direct access by the OS. All update
+requests must pass the sanity checks. (Parts of) the firmware must
+run with a higher priviledge level than the OS so this can be enforced
+by the firmware. On x86 this has been implemented using System
+Management Mode (SMM) in qemu and kvm, which again is the same
+approach taken by physical hardware. Only priviedged code running in
+SMM mode is allowed to access flash storage.
+
+Communication with the firmware code running in SMM mode works by
+serializing the requests to a shared buffer, then trapping into SMM
+mode via SMI. The SMM code processes the request, stores the reply in
+the same buffer and returns.
+
+Host UEFI variable service
+==========================
+
+Instead of running the priviledged code inside the guest we can run it
+on the host. The serialization protocol cen be reused. The
+communication with the host uses a virtual device, which essentially
+allows to configure the shared buffer location and size and to trap to
+the host to process the requests.
+
+The ``uefi-vars`` device implements the UEFI virtual device. It comes
+in ``uefi-vars-isa`` and ``uefi-vars-sysbus`` flavours. The device
+reimplements the handlers needed, specifically
+``EfiSmmVariableProtocol`` and ``VarCheckPolicyLibMmiHandler``. It
+also consumes events (``EfiEndOfDxeEventGroup``,
+``EfiEventReadyToBoot`` and ``EfiEventExitBootServices``).
+
+The advantage of the approach is that we do not need a special
+prividge level for the firmware to protect itself, i.e. it does not
+depend on SMM emulation on x64, which allows to remove a bunch of
+complex code for SMM emulation from the linux kernel
+(CONFIG_KVM_SMM=n). It also allows to support secure boot on arm
+without implementing secure world (el3) emulation in kvm.
+
+Of course there are also downsides. The added device increases the
+attack surface of the host, and we are adding some code duplication
+because we have to reimplement some edk2 functionality in qemu.
+
+usage on x86_64 (isa)
+---------------------
+
+.. code::
+
+ qemu-system-x86_64 -device uefi-vars-isa,jsonfile=/path/to/vars.json
+
+usage on aarch64 (sysbus)
+-------------------------
+
+.. code::
+
+ qemu-system-aarch64 -M virt,x-uefi-vars=on
diff --git a/hw/uefi/TODO.md b/hw/uefi/TODO.md
new file mode 100644
index 000000000000..5d1cd15a798e
--- /dev/null
+++ b/hw/uefi/TODO.md
@@ -0,0 +1,17 @@
+
+uefi variable service - todo list
+---------------------------------
+
+* implement reading/writing variable update time.
+* implement authenticated variable updates.
+ - used for 'dbx' updates.
+
+known issues and limitations
+----------------------------
+
+* secure boot variables are read-only
+ - due to auth vars not being implemented yet.
+* works only on little endian hosts
+ - accessing structs in guest ram is done without endian conversion.
+* works only for 64-bit guests
+ - UINTN is mapped to uint64_t, for 32-bit guests that would be uint32_t
--
2.41.0
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 16/16] docs: add uefi variable service documentation and TODO list.
2023-11-15 15:12 ` [PATCH 16/16] docs: add uefi variable service documentation and TODO list Gerd Hoffmann
@ 2023-11-15 15:56 ` Eric Blake
0 siblings, 0 replies; 32+ messages in thread
From: Eric Blake @ 2023-11-15 15:56 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: qemu-devel, qemu-arm, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé, graf,
Philippe Mathieu-Daudé, Markus Armbruster
On Wed, Nov 15, 2023 at 04:12:38PM +0100, Gerd Hoffmann wrote:
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
> docs/devel/index-internals.rst | 1 +
> docs/devel/uefi-vars.rst | 66 ++++++++++++++++++++++++++++++++++
> hw/uefi/TODO.md | 17 +++++++++
> 3 files changed, 84 insertions(+)
> create mode 100644 docs/devel/uefi-vars.rst
> create mode 100644 hw/uefi/TODO.md
> +
> +Guest UEFI variable management
> +==============================
> +
> +Traditional approach for UEFI Variable storage in qemu guests is to
The traditional
> +work as close as possible to physical hardware. That means provide
providing
> +pflash as storage and leave the management of variables and flash to
leaving
> +the guest.
> +
> +Secure boot support comes with the requirement that the UEFI variable
> +storage must be protected against direct access by the OS. All update
> +requests must pass the sanity checks. (Parts of) the firmware must
> +run with a higher priviledge level than the OS so this can be enforced
privilege
> +by the firmware. On x86 this has been implemented using System
> +Management Mode (SMM) in qemu and kvm, which again is the same
> +approach taken by physical hardware. Only priviedged code running in
privileged
> +SMM mode is allowed to access flash storage.
> +
> +Communication with the firmware code running in SMM mode works by
> +serializing the requests to a shared buffer, then trapping into SMM
> +mode via SMI. The SMM code processes the request, stores the reply in
> +the same buffer and returns.
> +
> +Host UEFI variable service
> +==========================
> +
> +Instead of running the priviledged code inside the guest we can run it
privileged
> +on the host. The serialization protocol cen be reused. The
can
> +communication with the host uses a virtual device, which essentially
> +allows to configure the shared buffer location and size and to trap to
s/allows to configure/configures/
s/and to trap/, and traps/
> +the host to process the requests.
> +
> +The ``uefi-vars`` device implements the UEFI virtual device. It comes
> +in ``uefi-vars-isa`` and ``uefi-vars-sysbus`` flavours. The device
> +reimplements the handlers needed, specifically
> +``EfiSmmVariableProtocol`` and ``VarCheckPolicyLibMmiHandler``. It
> +also consumes events (``EfiEndOfDxeEventGroup``,
> +``EfiEventReadyToBoot`` and ``EfiEventExitBootServices``).
> +
> +The advantage of the approach is that we do not need a special
> +prividge level for the firmware to protect itself, i.e. it does not
privilege
> +depend on SMM emulation on x64, which allows to remove a bunch of
s/allows to remove/allows the removal of/
> +complex code for SMM emulation from the linux kernel
> +(CONFIG_KVM_SMM=n). It also allows to support secure boot on arm
s/to support/support for/
> +without implementing secure world (el3) emulation in kvm.
> +
> +Of course there are also downsides. The added device increases the
> +attack surface of the host, and we are adding some code duplication
> +because we have to reimplement some edk2 functionality in qemu.
> +
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-15 15:12 [PATCH 00/16] hw/uefi: add uefi variable service Gerd Hoffmann
` (15 preceding siblings ...)
2023-11-15 15:12 ` [PATCH 16/16] docs: add uefi variable service documentation and TODO list Gerd Hoffmann
@ 2023-11-20 11:53 ` Alexander Graf
2023-11-20 16:50 ` Gerd Hoffmann
16 siblings, 1 reply; 32+ messages in thread
From: Alexander Graf @ 2023-11-20 11:53 UTC (permalink / raw)
To: Gerd Hoffmann, qemu-devel
Cc: qemu-arm, Eric Blake, Thomas Huth, Michael Roth, Paolo Bonzini,
Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Markus Armbruster, Ard Biesheuvel,
mimoja
Hey Gerd!
On 15.11.23 16:12, Gerd Hoffmann wrote:
> This patch adds a virtual device to qemu which the uefi firmware can use
> to store variables. This moves the UEFI variable management from
> privileged guest code (managing vars in pflash) to the host. Main
> advantage is that the need to have privilege separation in the guest
> goes away.
>
> On x86 privileged guest code runs in SMM. It's supported by kvm, but
> not liked much by various stakeholders in cloud space due to the
> complexity SMM emulation brings.
>
> On arm privileged guest code runs in el3 (aka secure world). This is
> not supported by kvm, which is unlikely to change anytime soon given
> that even el2 support (nested virt) is being worked on for years and is
> not yet in mainline.
>
> The design idea is to reuse the request serialization protocol edk2 uses
> for communication between SMM and non-SMM code, so large chunks of the
> edk2 variable driver stack can be used unmodified. Only the driver
> which traps into SMM mode must be replaced by a driver which talks to
> qemu instead.
I'm not sure I like the split :). If we cut things off at the SMM
communication layer, we still have a lot of code inside the Runtime
Services (RTS) code that is edk2 specific which means we're tying
ourselves tightly to the edk2 data format. It also means we can not
easily expose UEFI variables that QEMU owns, which can come in very
handy when implementing MOR - another feature that depends on SMM today.
In EC2, we are simply serializing all variable RTS calls to the
hypervisor, similar to the Xen implementation
(https://www.youtube.com/watch?v=jiR8khaECEk).
The edk2 side code we have built is here:
https://github.com/aws/uefi/tree/main/edk2-stable202211 (see anything
with VarStore in the name).
For the vmm side, we currently have an AWS-internal C++ implementation
that I can convert into QEMU code and send as patch if there is real
interest. Given that we deal with untrusted input, I would strongly
prefer if we could move it to a Rust implementation instead though. We
started a Rust reimplementation of it that interface that can build as a
library with C bindings which QEMU could then link against:
https://github.com/Mimoja/rs-uefi-varstore/tree/for_main
The code never went beyond the initial stages, but if we're pulling the
variable store to QEMU, this would be the best path forward IMHO.
If instead, we just want something we can quickly integrate while eating
up the additional security risk, I think we should just reuse the Xen
implementation.
Alex
Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-20 11:53 ` [PATCH 00/16] hw/uefi: add uefi variable service Alexander Graf
@ 2023-11-20 16:50 ` Gerd Hoffmann
2023-11-21 15:58 ` Laszlo Ersek
0 siblings, 1 reply; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-20 16:50 UTC (permalink / raw)
To: Alexander Graf
Cc: qemu-devel, qemu-arm, Eric Blake, Thomas Huth, Michael Roth,
Paolo Bonzini, Peter Maydell, Marc-André Lureau,
László Érsek, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Markus Armbruster, Ard Biesheuvel,
mimoja
On Mon, Nov 20, 2023 at 12:53:45PM +0100, Alexander Graf wrote:
> Hey Gerd!
>
> On 15.11.23 16:12, Gerd Hoffmann wrote:
> > This patch adds a virtual device to qemu which the uefi firmware can use
> > to store variables. This moves the UEFI variable management from
> > privileged guest code (managing vars in pflash) to the host. Main
> > advantage is that the need to have privilege separation in the guest
> > goes away.
> >
> > On x86 privileged guest code runs in SMM. It's supported by kvm, but
> > not liked much by various stakeholders in cloud space due to the
> > complexity SMM emulation brings.
> >
> > On arm privileged guest code runs in el3 (aka secure world). This is
> > not supported by kvm, which is unlikely to change anytime soon given
> > that even el2 support (nested virt) is being worked on for years and is
> > not yet in mainline.
> >
> > The design idea is to reuse the request serialization protocol edk2 uses
> > for communication between SMM and non-SMM code, so large chunks of the
> > edk2 variable driver stack can be used unmodified. Only the driver
> > which traps into SMM mode must be replaced by a driver which talks to
> > qemu instead.
>
>
> I'm not sure I like the split :). If we cut things off at the SMM
> communication layer, we still have a lot of code inside the Runtime Services
> (RTS) code that is edk2 specific which means we're tying ourselves tightly
> to the edk2 data format.
Which data format?
Request serialization format? Yes. I can't see what is wrong with
that. We need one anyway, and I don't see why inventing a new one is
any better than the one we already have in edk2.
Variable storage format? Nope, that is not the case. The variable
driver supports a cache, which I think is a read-only mapping of the
variable store, so using that might imply we have to use edk2 storage
format. Didn't check in detail through. The cache is optional, can be
disabled at compile time using PcdEnableVariableRuntimeCache=FALSE, and
I intentionally do not use the cache feature, exactly to avoid unwanted
constrains to the host side implementation.
> It also means we can not easily expose UEFI
> variables that QEMU owns,
Qemu owning variables should be no problem. Adding monitor commands to
read/write UEFI variables should be possible too.
> which can come in very handy when implementing MOR
> - another feature that depends on SMM today.
Have a pointer for me? Google finds me
https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-guard-requirements,
which describes the variable behavior (which I think should be no
problem to implement), but doesn't say a word about what exactly gets
locked when enabled ...
> In EC2, we are simply serializing all variable RTS calls to the hypervisor,
The edk2 code effectively does the same (with PcdEnableVariableRuntimeCache=FALSE).
> similar to the Xen implementation
> (https://www.youtube.com/watch?v=jiR8khaECEk).
Is the Xen implementation upstream? Can't see a xen variable driver in
OvmfPkg. The video is from 2019. What is the state of this?
> The edk2 side code we have built is here:
> https://github.com/aws/uefi/tree/main/edk2-stable202211 (see anything with
> VarStore in the name).
Ok, so it's just the variables. The edk2 variant also sends variable
policy requests (see Edk2VariablePolicyProtocol,
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md).
And it can easily be extended should the need arise in the future (all
requests are tagged with a protocol/event guid).
> Given that we deal with untrusted input, I would strongly prefer if we could
> move it to a Rust implementation instead though.
Valid point.
I've started in C because I have next no to experience with rust (yet),
so getting a test-able / demo-able implementation done was much easier
for me. Also I think we don't have any rust infrastructure in qemu
(yet?). Being able to use the qapi / qobject support to read/write the
variable store in json format is nice too.
But I'm open to discuss other options.
> We started a Rust
> reimplementation of it that interface that can build as a library with C
> bindings which QEMU could then link against:
>
> https://github.com/Mimoja/rs-uefi-varstore/tree/for_main
>
> The code never went beyond the initial stages,
Hmm. Why not?
> but if we're pulling the variable store to QEMU, this would be the
> best path forward IMHO.
Ok, so you are trying to sell me a prototype as solution?
/me looks a bit skeptical ...
take care,
Gerd
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-20 16:50 ` Gerd Hoffmann
@ 2023-11-21 15:58 ` Laszlo Ersek
2023-11-21 16:08 ` Daniel P. Berrangé
2023-11-22 12:11 ` Gerd Hoffmann
0 siblings, 2 replies; 32+ messages in thread
From: Laszlo Ersek @ 2023-11-21 15:58 UTC (permalink / raw)
To: Gerd Hoffmann, Alexander Graf
Cc: qemu-devel, qemu-arm, Eric Blake, Thomas Huth, Michael Roth,
Paolo Bonzini, Peter Maydell, Marc-André Lureau,
Daniel P. Berrangé, Philippe Mathieu-Daudé,
Markus Armbruster, Ard Biesheuvel, mimoja
On 11/20/23 17:50, Gerd Hoffmann wrote:
> On Mon, Nov 20, 2023 at 12:53:45PM +0100, Alexander Graf wrote:
>> Hey Gerd!
>>
>> On 15.11.23 16:12, Gerd Hoffmann wrote:
>>> This patch adds a virtual device to qemu which the uefi firmware can use
>>> to store variables. This moves the UEFI variable management from
>>> privileged guest code (managing vars in pflash) to the host. Main
>>> advantage is that the need to have privilege separation in the guest
>>> goes away.
>>>
>>> On x86 privileged guest code runs in SMM. It's supported by kvm, but
>>> not liked much by various stakeholders in cloud space due to the
>>> complexity SMM emulation brings.
>>>
>>> On arm privileged guest code runs in el3 (aka secure world). This is
>>> not supported by kvm, which is unlikely to change anytime soon given
>>> that even el2 support (nested virt) is being worked on for years and is
>>> not yet in mainline.
>>>
>>> The design idea is to reuse the request serialization protocol edk2 uses
>>> for communication between SMM and non-SMM code, so large chunks of the
>>> edk2 variable driver stack can be used unmodified. Only the driver
>>> which traps into SMM mode must be replaced by a driver which talks to
>>> qemu instead.
>>
>>
>> I'm not sure I like the split :). If we cut things off at the SMM
>> communication layer, we still have a lot of code inside the Runtime Services
>> (RTS) code that is edk2 specific which means we're tying ourselves tightly
>> to the edk2 data format.
>
> Which data format?
>
> Request serialization format? Yes. I can't see what is wrong with
> that.
... I can't even see what's wrong with *that*. Realistically /
practically, I think only edk2 has been considered as guest UEFI
firmware for QEMU/KVM virtual machines, as far as upstream projects go.
It's certainly edk2 that's bundled with QEMU.
My understanding is that firmware is just considered a part of the
virtualization platform, so teaching edk2 specifics to QEMU doesn't seem
wrong. (As long as we have the personpower to maintain interoperability.)
> We need one anyway, and I don't see why inventing a new one is
> any better than the one we already have in edk2.
>
> Variable storage format? Nope, that is not the case. The variable
> driver supports a cache, which I think is a read-only mapping of the
> variable store, so using that might imply we have to use edk2 storage
> format. Didn't check in detail through. The cache is optional, can be
> disabled at compile time using PcdEnableVariableRuntimeCache=FALSE, and
> I intentionally do not use the cache feature, exactly to avoid unwanted
> constrains to the host side implementation.
>
>> It also means we can not easily expose UEFI
>> variables that QEMU owns,
>
> Qemu owning variables should be no problem. Adding monitor commands to
> read/write UEFI variables should be possible too.
This patch set is actually an improvement towards QEMU-owned variables.
Right now, all variables are internal to the guest; QEMU only has a
pflash-level view.
>
>> which can come in very handy when implementing MOR
>> - another feature that depends on SMM today.
>
> Have a pointer for me? Google finds me
> https://learn.microsoft.com/en-us/windows-hardware/drivers/bringup/device-guard-requirements,
> which describes the variable behavior (which I think should be no
> problem to implement), but doesn't say a word about what exactly gets
> locked when enabled ...
See:
TCG PC Client Platform
Reset Attack Mitigation Specification
My copy is
Family “2.0”
Version 1.10 Revision 17
January 21, 2019
Published
You should find it somewhere in the download area of
<https://trustedcomputinggroup.org>.
E.g....
<https://www.trustedcomputinggroup.org/wp-content/uploads/Platform-Reset-Attack-Mitigation-Specification.pdf>
In the past we've had a bunch of discussions / patches around this. Some
examples:
- [edk2] multiple levels of support for MOR / MORLock
http://mid.mail-archive.com/039cf353-80fb-9f20-6ad2-f52517ab4de7@redhat.com
- https://bugzilla.tianocore.org/show_bug.cgi?id=727
(see edk2 commit range listed there, too)
- commit 704b71d7e11f115a3b5b03471d6420a7a70f1585
- commit d20ae95a13e851d56c6618108b18c93526505ca2
- https://bugzilla.redhat.com/show_bug.cgi?id=1854212
- https://bugzilla.redhat.com/show_bug.cgi?id=1498159
>
>> In EC2, we are simply serializing all variable RTS calls to the hypervisor,
>
> The edk2 code effectively does the same (with PcdEnableVariableRuntimeCache=FALSE).
>
>> similar to the Xen implementation
>> (https://www.youtube.com/watch?v=jiR8khaECEk).
>
> Is the Xen implementation upstream? Can't see a xen variable driver in
> OvmfPkg. The video is from 2019. What is the state of this?
Not sure about the current state, but when that presentation came out,
we discussed it briefly internally. I don't have time to review that old
discussion now for the sake of potentially publishing it inside this
discussion, but for interested Red Hatters, I can provide a pointer:
[virt-devel] Securing Secure Boot on Xen | FOSDEM'19
Date: Fri, 8 Feb 2019 20:46:46 +0100
msgid: <37382312-986c-4ce4-1ba3-942697738d65@redhat.com>
>
>> The edk2 side code we have built is here:
>> https://github.com/aws/uefi/tree/main/edk2-stable202211 (see anything with
>> VarStore in the name).
Well... why was this never upstreamed to edk2? (Side question, of course.)
>
> Ok, so it's just the variables. The edk2 variant also sends variable
> policy requests (see Edk2VariablePolicyProtocol,
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/VariablePolicyLib/ReadMe.md).
> And it can easily be extended should the need arise in the future (all
> requests are tagged with a protocol/event guid).
>
>> Given that we deal with untrusted input, I would strongly prefer if we could
>> move it to a Rust implementation instead though.
>
> Valid point.
Agree that the point is valid.
(And this was one of my concerns, if not my main concern, in the above
internal discussion, about Xen varstored.)
Especially due to the heavy crypto that the final version is supposed to
have.
Even during the present patch review, while going through only the
headers thus far, I've already said at least twice that we're going to
have to be super careful about integer overflows and buffer overflows.
Any such problem is no longer a guest<->guest privilege boundary breach
but a guest<->host one.
Not sure if the suggested remedy ("write it in Rust") is practical.
>
> I've started in C because I have next no to experience with rust (yet),
> so getting a test-able / demo-able implementation done was much easier
> for me. Also I think we don't have any rust infrastructure in qemu
> (yet?). Being able to use the qapi / qobject support to read/write the
> variable store in json format is nice too.
>
> But I'm open to discuss other options.
A selfish aspect: given that I've been reviewing this set, should I
consider it a proof of concept / prototype, or something we might want
to build upon, i.e., should I assume we'd put these foundations into
production at some point? I've been reviewing the series with the latter
in mind, but if that's not correct, I should probably adjust my pedantry
knob.
>
>> We started a Rust
>> reimplementation of it that interface that can build as a library with C
>> bindings which QEMU could then link against:
>>
>> https://github.com/Mimoja/rs-uefi-varstore/tree/for_main
>>
>> The code never went beyond the initial stages,
>
> Hmm. Why not?
And I'm a bit annoyed that you had to write ~2500 lines of QEMU code
(not counting edk2) for qemu-devel to learn about these initiatives. :/
>
>> but if we're pulling the variable store to QEMU, this would be the
>> best path forward IMHO.
>
> Ok, so you are trying to sell me a prototype as solution?
> /me looks a bit skeptical ...
at least with virtiofsd, we had gone with a C impl first, and only then
with a Rust impl...
Laszlo
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-21 15:58 ` Laszlo Ersek
@ 2023-11-21 16:08 ` Daniel P. Berrangé
2023-11-22 12:40 ` Gerd Hoffmann
2023-11-22 12:11 ` Gerd Hoffmann
1 sibling, 1 reply; 32+ messages in thread
From: Daniel P. Berrangé @ 2023-11-21 16:08 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Gerd Hoffmann, Alexander Graf, qemu-devel, qemu-arm, Eric Blake,
Thomas Huth, Michael Roth, Paolo Bonzini, Peter Maydell,
Marc-André Lureau, Philippe Mathieu-Daudé,
Markus Armbruster, Ard Biesheuvel, mimoja
On Tue, Nov 21, 2023 at 04:58:44PM +0100, Laszlo Ersek wrote:
> On 11/20/23 17:50, Gerd Hoffmann wrote:
> > On Mon, Nov 20, 2023 at 12:53:45PM +0100, Alexander Graf wrote:
> >> Hey Gerd!
> >>
> >> On 15.11.23 16:12, Gerd Hoffmann wrote:
> >>> This patch adds a virtual device to qemu which the uefi firmware can use
> >>> to store variables. This moves the UEFI variable management from
> >>> privileged guest code (managing vars in pflash) to the host. Main
> >>> advantage is that the need to have privilege separation in the guest
> >>> goes away.
> >>>
> >>> On x86 privileged guest code runs in SMM. It's supported by kvm, but
> >>> not liked much by various stakeholders in cloud space due to the
> >>> complexity SMM emulation brings.
> >>>
> >>> On arm privileged guest code runs in el3 (aka secure world). This is
> >>> not supported by kvm, which is unlikely to change anytime soon given
> >>> that even el2 support (nested virt) is being worked on for years and is
> >>> not yet in mainline.
> >>>
> >>> The design idea is to reuse the request serialization protocol edk2 uses
> >>> for communication between SMM and non-SMM code, so large chunks of the
> >>> edk2 variable driver stack can be used unmodified. Only the driver
> >>> which traps into SMM mode must be replaced by a driver which talks to
> >>> qemu instead.
> >>
> >>
> >> I'm not sure I like the split :). If we cut things off at the SMM
> >> communication layer, we still have a lot of code inside the Runtime Services
> >> (RTS) code that is edk2 specific which means we're tying ourselves tightly
> >> to the edk2 data format.
> >
> > Which data format?
> >
> > Request serialization format? Yes. I can't see what is wrong with
> > that.
>
> ... I can't even see what's wrong with *that*. Realistically /
> practically, I think only edk2 has been considered as guest UEFI
> firmware for QEMU/KVM virtual machines, as far as upstream projects go.
> It's certainly edk2 that's bundled with QEMU.
>
> My understanding is that firmware is just considered a part of the
> virtualization platform, so teaching edk2 specifics to QEMU doesn't seem
> wrong. (As long as we have the personpower to maintain interoperability.)
>
> > We need one anyway, and I don't see why inventing a new one is
> > any better than the one we already have in edk2.
> >
> > Variable storage format? Nope, that is not the case. The variable
> > driver supports a cache, which I think is a read-only mapping of the
> > variable store, so using that might imply we have to use edk2 storage
> > format. Didn't check in detail through. The cache is optional, can be
> > disabled at compile time using PcdEnableVariableRuntimeCache=FALSE, and
> > I intentionally do not use the cache feature, exactly to avoid unwanted
> > constrains to the host side implementation.
> >
> >> It also means we can not easily expose UEFI
> >> variables that QEMU owns,
> >
> > Qemu owning variables should be no problem. Adding monitor commands to
> > read/write UEFI variables should be possible too.
>
> This patch set is actually an improvement towards QEMU-owned variables.
> Right now, all variables are internal to the guest; QEMU only has a
> pflash-level view.
To throw confidental computing into the mix....
Right now for SEV-SNP/TDX, the EDK2 builds are stateless so that
variables are thrown away at poweroff.
Long term though there's interest in having the ability to (optionally)
offer persistence of variables to confidential computing too.
The key issue is that the host QEMU is not trusted so it would not be
viable to let QEMU receive the variables in plain text.
One option I've illustrated before is that have SVSM (or equiv)
expose an encrypted storage service to EDK2. Given the proposed EDK2
side protocol/modifications for variable storage, I wonder if it is
viable for SVSM (or equiv) to replace QEMU in providing the backend
storage impl ? IOW, host QEMU would expose a pflash to the guest,
to which SVSM would apply full disk encryption, and then store the
EFI variables in it
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-21 16:08 ` Daniel P. Berrangé
@ 2023-11-22 12:40 ` Gerd Hoffmann
0 siblings, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-22 12:40 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laszlo Ersek, Alexander Graf, qemu-devel, qemu-arm, Eric Blake,
Thomas Huth, Michael Roth, Paolo Bonzini, Peter Maydell,
Marc-André Lureau, Philippe Mathieu-Daudé,
Markus Armbruster, Ard Biesheuvel, mimoja
Hi,
> One option I've illustrated before is that have SVSM (or equiv)
> expose an encrypted storage service to EDK2. Given the proposed EDK2
> side protocol/modifications for variable storage, I wonder if it is
> viable for SVSM (or equiv) to replace QEMU in providing the backend
> storage impl ? IOW, host QEMU would expose a pflash to the guest,
> to which SVSM would apply full disk encryption, and then store the
> EFI variables in it
Yes. IIRC (it's been a while I've looked at the spec) the SVSM can
request that access to specific pages trap, so it could use that to
emulate the mmio/sysbus variant of the device. Not sure if that could
work for io/isa too. Another option would be to add a third interface
variant which uses SVSM service calls.
And one advantage of a rust implementation would be that integrating
with coconut-svsm (which is written in rust too) might be easier. On
the other hand the SVSM is a very different environment, the rust stdlib
is not available for example, and the way persistence is implemented
will probably look very different too. Another difference is crypto
support, qemu uses nettle / gnutls whereas svsm will probably use
openssl. So not sure how big the opportunity to share the code between
qemu and svsm on the backend side actually is.
take care,
Gerd
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 00/16] hw/uefi: add uefi variable service
2023-11-21 15:58 ` Laszlo Ersek
2023-11-21 16:08 ` Daniel P. Berrangé
@ 2023-11-22 12:11 ` Gerd Hoffmann
1 sibling, 0 replies; 32+ messages in thread
From: Gerd Hoffmann @ 2023-11-22 12:11 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Alexander Graf, qemu-devel, qemu-arm, Eric Blake, Thomas Huth,
Michael Roth, Paolo Bonzini, Peter Maydell,
Marc-André Lureau, Daniel P. Berrangé,
Philippe Mathieu-Daudé, Markus Armbruster, Ard Biesheuvel,
mimoja
Hi,
> Even during the present patch review, while going through only the
> headers thus far, I've already said at least twice that we're going to
> have to be super careful about integer overflows and buffer overflows.
> Any such problem is no longer a guest<->guest privilege boundary breach
> but a guest<->host one.
>
> Not sure if the suggested remedy ("write it in Rust") is practical.
It should prevent certain classes of bugs such as buffer overflows. Not
sure how much compromises you have to make (i.e. 'unsafe' code sections)
for a C library interface, so you can link the lib into qemu. And of
course it wouldn't automatically stop logic errors.
> > But I'm open to discuss other options.
>
> A selfish aspect: given that I've been reviewing this set, should I
> consider it a proof of concept / prototype, or something we might want
> to build upon, i.e., should I assume we'd put these foundations into
> production at some point? I've been reviewing the series with the latter
> in mind, but if that's not correct, I should probably adjust my pedantry
> knob.
In case we continue the C route I certainly expect that this patch set
will turn into something production-ready, and I've tried to code things
up accordingly. Copy buffers so the guest can't modify them while qemu
processes them, carefully check length fields, ...
> at least with virtiofsd, we had gone with a C impl first, and only then
> with a Rust impl...
And virtiofsd was easier because it is a completely separate process,
not something you want link into qemu ...
take care,
Gerd
^ permalink raw reply [flat|nested] 32+ messages in thread