On 03/11/19 19:27, Philippe Mathieu-Daudé wrote: > On 3/11/19 8:26 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <phi...@redhat.com> writes: >> >>> The Edk2Crypto object is used to hold configuration values specific >>> to EDK2. >>> >>> The edk2_add_host_crypto_policy() function loads crypto policies >>> from the host, and register them as fw_cfg named file items. >>> So far only the 'https' policy is supported. >>> >>> A usercase example is the 'HTTPS Boof' feature of OVMF [*]. >>> >>> Usage example: >>> >>> $ qemu-system-x86_64 \ >>> --object edk2_crypto,id=https,\ >>> ciphers=/etc/crypto-policies/back-ends/openssl.config,\ >>> cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin >>> >>> (On Fedora these files are provided by the ca-certificates and >>> crypto-policies packages). >>> >>> [*]: https://github.com/tianocore/edk2/blob/master/OvmfPkg/README >>> >>> Signed-off-by: Philippe Mathieu-Daudé <phi...@redhat.com> >>> --- >>> v3: >>> - '-object' -> '--object' in commit description (Eric) >>> - reworded the 'TODO: g_free' comment >>> --- >>> MAINTAINERS | 8 ++ >>> hw/Makefile.objs | 1 + >>> hw/firmware/Makefile.objs | 1 + >>> hw/firmware/uefi_edk2_crypto_policies.c | 177 ++++++++++++++++++++++++ >>> include/hw/firmware/uefi_edk2.h | 28 ++++ >>> 5 files changed, 215 insertions(+) >>> create mode 100644 hw/firmware/Makefile.objs >>> create mode 100644 hw/firmware/uefi_edk2_crypto_policies.c >>> create mode 100644 include/hw/firmware/uefi_edk2.h >>> >>> diff --git a/MAINTAINERS b/MAINTAINERS >>> index cf09a4c127..70122b3d0d 100644 >>> --- a/MAINTAINERS >>> +++ b/MAINTAINERS >>> @@ -2206,6 +2206,14 @@ F: include/hw/i2c/smbus_master.h >>> F: include/hw/i2c/smbus_slave.h >>> F: include/hw/i2c/smbus_eeprom.h >>> >>> +EDK2 Firmware >>> +M: Laszlo Ersek <ler...@redhat.com> >>> +M: Philippe Mathieu-Daudé <phi...@redhat.com> >>> +S: Maintained >>> +F: docs/interop/firmware.json >>> +F: hw/firmware/uefi_edk2_crypto_policies.c >>> +F: include/hw/firmware/uefi_edk2.h >>> + >>> Usermode Emulation >>> ------------------ >>> Overall >>> diff --git a/hw/Makefile.objs b/hw/Makefile.objs >>> index 82aa7fab8e..2b075aa1e0 100644 >>> --- a/hw/Makefile.objs >>> +++ b/hw/Makefile.objs >>> @@ -8,6 +8,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += char/ >>> devices-dirs-$(CONFIG_SOFTMMU) += cpu/ >>> devices-dirs-$(CONFIG_SOFTMMU) += display/ >>> devices-dirs-$(CONFIG_SOFTMMU) += dma/ >>> +devices-dirs-$(CONFIG_SOFTMMU) += firmware/ >>> devices-dirs-$(CONFIG_SOFTMMU) += gpio/ >>> devices-dirs-$(CONFIG_HYPERV) += hyperv/ >>> devices-dirs-$(CONFIG_I2C) += i2c/ >>> diff --git a/hw/firmware/Makefile.objs b/hw/firmware/Makefile.objs >>> new file mode 100644 >>> index 0000000000..ea1f6d44df >>> --- /dev/null >>> +++ b/hw/firmware/Makefile.objs >>> @@ -0,0 +1 @@ >>> +common-obj-y += uefi_edk2_crypto_policies.o >>> diff --git a/hw/firmware/uefi_edk2_crypto_policies.c >>> b/hw/firmware/uefi_edk2_crypto_policies.c >>> new file mode 100644 >>> index 0000000000..5f88819a50 >>> --- /dev/null >>> +++ b/hw/firmware/uefi_edk2_crypto_policies.c >>> @@ -0,0 +1,177 @@ >>> +/* >>> + * UEFI EDK2 Support >>> + * >>> + * Copyright (c) 2019 Red Hat Inc. >>> + * >>> + * Author: >>> + * Philippe Mathieu-Daudé <phi...@redhat.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>> later. >>> + * See the COPYING file in the top-level directory. >>> + */ >>> + >>> +#include "qemu/osdep.h" >>> +#include "qapi/error.h" >>> +#include "qom/object_interfaces.h" >>> +#include "hw/firmware/uefi_edk2.h" >>> + >>> + >>> +#define TYPE_EDK2_CRYPTO "edk2_crypto" >>> + >>> +#define EDK2_CRYPTO_CLASS(klass) \ >>> + OBJECT_CLASS_CHECK(Edk2CryptoClass, (klass), \ >>> + TYPE_EDK2_CRYPTO) >>> +#define EDK2_CRYPTO_GET_CLASS(obj) \ >>> + OBJECT_GET_CLASS(Edk2CryptoClass, (obj), \ >>> + TYPE_EDK2_CRYPTO) >>> +#define EDK2_CRYPTO(obj) \ >>> + INTERFACE_CHECK(Edk2Crypto, (obj), \ >>> + TYPE_EDK2_CRYPTO) >> >> Uh, should this be OBJECT_CLASS_CHECK()? TYPE_EDK2_CRYPTO is a >> TYPE_OBJECT, not a TYPE_INTERFACE... > > Good catch! > >>> + >>> +typedef struct Edk2Crypto { >>> + Object parent_obj; >>> + >>> + /* >>> + * Path to the acceptable ciphersuites and the preferred order from >>> + * the host-side crypto policy. >>> + */ >>> + char *ciphers_path; >>> + >>> + /* Path to the trusted CA certificates configured on the host side. */ >>> + char *cacerts_path; >>> +} Edk2Crypto; >> >> Bike-shedding: I prefer to call file names file names, and reserve >> "path" for search paths. But it's your shed, not mine. > > OK.
I prefer "pathname" and "filename": http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_271 http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap03.html#tag_03_170 In this case, I think "pathname" applies. I agree that "path" is somewhat unfortunate. > >>> + >>> +typedef struct Edk2CryptoClass { >>> + ObjectClass parent_class; >>> +} Edk2CryptoClass; >>> + >>> + >>> +static void edk2_crypto_prop_set_ciphers(Object *obj, const char *value, >>> + Error **errp G_GNUC_UNUSED) >>> +{ >>> + Edk2Crypto *s = EDK2_CRYPTO(obj); >>> + >>> + g_free(s->ciphers_path); >>> + s->ciphers_path = g_strdup(value); >>> +} >>> + >>> +static char *edk2_crypto_prop_get_ciphers(Object *obj, >>> + Error **errp G_GNUC_UNUSED) >>> +{ >>> + Edk2Crypto *s = EDK2_CRYPTO(obj); >>> + >>> + return g_strdup(s->ciphers_path); >>> +} >>> + >>> +static void edk2_crypto_prop_set_cacerts(Object *obj, const char *value, >>> + Error **errp G_GNUC_UNUSED) >>> +{ >>> + Edk2Crypto *s = EDK2_CRYPTO(obj); >>> + >>> + g_free(s->cacerts_path); >>> + s->cacerts_path = g_strdup(value); >>> +} >>> + >>> +static char *edk2_crypto_prop_get_cacerts(Object *obj, >>> + Error **errp G_GNUC_UNUSED) >>> +{ >>> + Edk2Crypto *s = EDK2_CRYPTO(obj); >>> + >>> + return g_strdup(s->cacerts_path); >>> +} >>> + >>> +static void edk2_crypto_finalize(Object *obj) >>> +{ >>> + Edk2Crypto *s = EDK2_CRYPTO(obj); >>> + >>> + g_free(s->ciphers_path); >>> + g_free(s->cacerts_path); >>> +} >>> + >>> +static void edk2_crypto_class_init(ObjectClass *oc, void *data) >>> +{ >>> + object_class_property_add_str(oc, "ciphers", >>> + edk2_crypto_prop_get_ciphers, >>> + edk2_crypto_prop_set_ciphers, >>> + NULL); >>> + object_class_property_add_str(oc, "cacerts", >>> + edk2_crypto_prop_get_cacerts, >>> + edk2_crypto_prop_set_cacerts, >>> + NULL); >>> +} >>> + >>> +static const TypeInfo edk2_crypto_info = { >>> + .parent = TYPE_OBJECT, >>> + .name = TYPE_EDK2_CRYPTO, >>> + .instance_size = sizeof(Edk2Crypto), >>> + .instance_finalize = edk2_crypto_finalize, >>> + .class_size = sizeof(Edk2CryptoClass), >>> + .class_init = edk2_crypto_class_init, >>> + .interfaces = (InterfaceInfo[]) { >>> + { TYPE_USER_CREATABLE }, >>> + { } >>> + } >>> +}; >>> + >>> +static void edk2_crypto_register_types(void) >>> +{ >>> + type_register_static(&edk2_crypto_info); >>> +} >>> + >>> +type_init(edk2_crypto_register_types); >>> + >>> +static Edk2Crypto *edk2_crypto_by_id(const char *edk_crypto_id, Error >>> **errp) >>> +{ >>> + Object *obj; >>> + Object *container; >>> + >>> + container = object_get_objects_root(); >>> + obj = object_resolve_path_component(container, >>> + edk_crypto_id); >>> + if (!obj) { >>> + error_setg(errp, "Cannot find EDK2 crypto object ID %s", >>> + edk_crypto_id); >>> + return NULL; >>> + } >>> + >>> + if (!object_dynamic_cast(obj, TYPE_EDK2_CRYPTO)) { >>> + error_setg(errp, "Object '%s' is not a EDK2 crypto subclass", >>> + edk_crypto_id); >>> + return NULL; >>> + } >>> + >>> + return EDK2_CRYPTO(obj); >>> +} >>> + >>> +void edk2_add_host_crypto_policy(FWCfgState *fw_cfg) >>> +{ >>> + Edk2Crypto *s; >>> + >>> + s = edk2_crypto_by_id("https", NULL); >>> + if (!s) { >>> + return; >>> + } >>> + >>> + if (s->ciphers_path) { >>> + /* >>> + * Note: >>> + * Unlike with fw_cfg_add_file() where the allocated data has >>> + * to be valid for the lifetime of the FwCfg object, there is >>> + * no such contract interface with fw_cfg_add_file_from_host(). >> >> In my review of PATCH 1, I argue there should be. >> >>> + * It would be easier that the FwCfg object keeps reference of >>> + * its allocated memory and releases it when destroy, but it >>> + * currently doesn't. Meanwhile we simply add this TODO comment. >>> + */ >>> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/ciphers", >>> + s->ciphers_path, NULL); >>> + } >>> + >>> + if (s->cacerts_path) { >>> + /* >>> + * TODO: g_free the returned pointer >>> + * (see previous comment for ciphers_path in this function). >>> + */ >> >> Is it even possible to reolve this TODO? Is there any point in time >> where we can free the returned pointer without leaving a dangling >> pointer in @fw_cfg? > > I understood Laszlo suggested the fw_cfg devices do the garbage collection. I wouldn't call it "garbage collection"; I'd rather say fw_cfg should ultimately own (= be responsible for destroying) all data items that it exposes to the guest. Assuming, of course, that we actually mean to make fw_cfg destroyable. (To me "garbage collection" is a language feature, and it has aspects that don't apply here, such as dealing with cycles of pointers, performance characteristics etc.) Thanks Laszlo > >>> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", >>> + s->cacerts_path, NULL); >>> + } >>> +} >>> diff --git a/include/hw/firmware/uefi_edk2.h >>> b/include/hw/firmware/uefi_edk2.h >>> new file mode 100644 >>> index 0000000000..e0b2fb160a >>> --- /dev/null >>> +++ b/include/hw/firmware/uefi_edk2.h >>> @@ -0,0 +1,28 @@ >>> +/* >>> + * UEFI EDK2 Support >>> + * >>> + * Copyright (c) 2019 Red Hat Inc. >>> + * >>> + * Author: >>> + * Philippe Mathieu-Daudé <phi...@redhat.com> >>> + * >>> + * This work is licensed under the terms of the GNU GPL, version 2 or >>> later. >>> + * See the COPYING file in the top-level directory. >>> + */ >>> + >>> +#ifndef HW_FIRMWARE_UEFI_EDK2_H >>> +#define HW_FIRMWARE_UEFI_EDK2_H >>> + >>> +#include "hw/nvram/fw_cfg.h" >>> + >>> +/** >>> + * edk2_add_host_crypto_policy: >>> + * @s: fw_cfg device being modified >>> + * >>> + * Add a new named file containing the host crypto policy. >>> + * >>> + * Currently only the 'https' policy is supported. >>> + */ >>> +void edk2_add_host_crypto_policy(FWCfgState *s); >> >> Out of curiosity, what happens if you call this more than once? > > Such curiosity is useful, thanks :) > >>> + >>> +#endif /* HW_FIRMWARE_UEFI_EDK2_H */
