| Message ID | 20190310004749.27029-3-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | fw_cfg: Add edk2_add_host_crypto_policy() | expand |
Philippe Mathieu-Daudé <philmd@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é <philmd@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 <lersek@redhat.com> > +M: Philippe Mathieu-Daudé <philmd@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é <philmd@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... > + > +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. > + > +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? > + 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é <philmd@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? > + > +#endif /* HW_FIRMWARE_UEFI_EDK2_H */
On 3/11/19 8:26 AM, Markus Armbruster wrote: > Philippe Mathieu-Daudé <philmd@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é <philmd@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 <lersek@redhat.com> >> +M: Philippe Mathieu-Daudé <philmd@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é <philmd@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. >> + >> +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. >> + 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é <philmd@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 */
On Mon, Mar 11, 2019 at 7:27 PM Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/11/19 8:26 AM, Markus Armbruster wrote: > > Philippe Mathieu-Daudé <philmd@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é <philmd@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 <lersek@redhat.com> > >> +M: Philippe Mathieu-Daudé <philmd@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é <philmd@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... OBJECT_CHECK() actually. > 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. > > >> + > >> +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. > > >> + 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é <philmd@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 */
On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: > 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é <philmd@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 <lersek@redhat.com> > +M: Philippe Mathieu-Daudé <philmd@redhat.com> > +S: Maintained > +F: docs/interop/firmware.json > +F: hw/firmware/uefi_edk2_crypto_policies.c > +F: include/hw/firmware/uefi_edk2.h > + I'm not happy with this. First, "docs/interop/firmware.json" is meant for more than just EDK2. We shouldn't list it in a section called "EDK2 Firmware". I can't suggest an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one would be misleading. Second, we expose fw_cfg files to edk2 platform firmware from a bunch of other places. For example -- and in this case I do mean to provide a complex example! --, see "etc/smi/supported-features", "etc/smi/requested-features", and "etc/smi/features-ok", in file "hw/isa/lpc_ich9.c". I'm unconvinced that the present feature merits new directories and new files. Then again, I also don't know where to put the logic. I guess I'll have to defer to more experienced reviewers. [snipping lots of QOM boilerplate] > +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(). > + * 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). > + */ > + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", > + s->cacerts_path, NULL); > + } > +} Shouldn't we do some error checking here? I mean, printing an error message in fw_cfg_add_file_from_host(), and then continuing without exposing the named files in question to the firmware, could be OK if this was a "default on" feature. But (IIUC) here the user provided an explicit "-object" option, and we've just failed to construct the object. Doesn't such a situation usually prevent QEMU startup? Thanks, Laszlo > 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é <philmd@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); > + > +#endif /* HW_FIRMWARE_UEFI_EDK2_H */ >
On 03/11/19 19:27, Philippe Mathieu-Daudé wrote: > On 3/11/19 8:26 AM, Markus Armbruster wrote: >> Philippe Mathieu-Daudé <philmd@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é <philmd@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 <lersek@redhat.com> >>> +M: Philippe Mathieu-Daudé <philmd@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é <philmd@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é <philmd@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 */
On 03/13/19 10:43, Laszlo Ersek wrote: > On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: >> 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é <philmd@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 <lersek@redhat.com> >> +M: Philippe Mathieu-Daudé <philmd@redhat.com> >> +S: Maintained >> +F: docs/interop/firmware.json >> +F: hw/firmware/uefi_edk2_crypto_policies.c >> +F: include/hw/firmware/uefi_edk2.h >> + > > I'm not happy with this. > > First, "docs/interop/firmware.json" is meant for more than just EDK2. We > shouldn't list it in a section called "EDK2 Firmware". I can't suggest > an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one > would be misleading. > > Second, we expose fw_cfg files to edk2 platform firmware from a bunch of > other places. For example -- and in this case I do mean to provide a > complex example! --, see "etc/smi/supported-features", > "etc/smi/requested-features", and "etc/smi/features-ok", in file > "hw/isa/lpc_ich9.c". I'm unconvinced that the present feature merits new > directories and new files. > > Then again, I also don't know where to put the logic. I guess I'll have > to defer to more experienced reviewers. > > [snipping lots of QOM boilerplate] > >> +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(). >> + * 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). >> + */ >> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", >> + s->cacerts_path, NULL); >> + } >> +} > > Shouldn't we do some error checking here? > > I mean, printing an error message in fw_cfg_add_file_from_host(), and > then continuing without exposing the named files in question to the > firmware, could be OK if this was a "default on" feature. But (IIUC) > here the user provided an explicit "-object" option, and we've just > failed to construct the object. Doesn't such a situation usually prevent > QEMU startup? Wait, I could be totally confused here. (Returning to this patch after seeing the rest of the series.) Is it actually the case that the Edk2Crypto object holds nothing more than two pathnames -- and so its construction can virtually never fail? While the actual fw_cfg population occurs separately, in a machine_done notifier? If that's the case, I don't think it's the right approach. Reading the host files, and populating fw_cfg with them, should be part of the object construction. And if those steps fail, the object should not be possible to construct. We did something similar with the vmgenid device [hw/acpi/vmgenid.c], if I remember correctly. It also has a dependency on fw_cfg... Ahh, no, I'm absolutely wrong about that. vmgenid_realize() doesn't do anything with fw_cfg. Instead, we have acpi_setup() in "hw/i386/acpi-build.c", which calls find_vmgenid_dev() and vmgenid_add_fw_cfg(). And acpi_setup() is certainly called from pc_machine_done(). In other words, the pattern that you use here matches existing practice. Realize the device (or object) first, then add the fw_cfg thingies in the "machine done" callback. OK. *Still*, I would like to see better error handling/reporting (or an explanation why I'm wrong). How about reworking the edk2crypto class itself -- it shouldn't just hold the pathnames of the files, but also their contents. That is: - g_file_get_contents() should be called in the realize method - the object would own the file contents - the realize method would ensure that there wouldn't be any other instance of the class (i.e. that it would be a singleton -- see the same idea in vmgenid) - there would be no need for the fw_cfg_add_file_from_host() API - the machine done notifier would be extended to locate the object (there could be zero or one instances), and if the one instance were found, the machine done callback would hook the file contents into fw_cfg. fw_cfg_add_file() cannot fail, so no errors to report at this stage. Again I think this would follow the pattern from vmgenid. Thanks, Laszlo >> 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é <philmd@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); >> + >> +#endif /* HW_FIRMWARE_UEFI_EDK2_H */ >> >
On Wed, Mar 13, 2019 at 10:43:29AM +0100, Laszlo Ersek wrote: > On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: > > 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é <philmd@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 <lersek@redhat.com> > > +M: Philippe Mathieu-Daudé <philmd@redhat.com> > > +S: Maintained > > +F: docs/interop/firmware.json > > +F: hw/firmware/uefi_edk2_crypto_policies.c > > +F: include/hw/firmware/uefi_edk2.h > > + > > I'm not happy with this. > > First, "docs/interop/firmware.json" is meant for more than just EDK2. We > shouldn't list it in a section called "EDK2 Firmware". I can't suggest > an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one > would be misleading. We can add arbitrary entries, so I'd would split the above into 2 sections Firmware specs M: Laszlo Ersek <lersek@redhat.com> M: Philippe Mathieu-Daudé <philmd@redhat.com> S: Maintained F: docs/interop/firmware.json EDK2 Firmware M: Laszlo Ersek <lersek@redhat.com> M: Philippe Mathieu-Daudé <philmd@redhat.com> S: Maintained F: hw/firmware/uefi_edk2_crypto_policies.c F: include/hw/firmware/uefi_edk2.h Regards, Daniel
On 03/13/19 11:11, Laszlo Ersek wrote:
> Again I think this would follow the pattern from vmgenid.
And then I'd agree that we should dedicate a separate file (perhaps even
a separate directory) to the new class.
(I understand I keep mixing up "object" with "device" -- sorry about
that. I hope my suggestion makes sense nonetheless.)
Thanks
Laszlo
On 3/13/19 11:11 AM, Laszlo Ersek wrote: > On 03/13/19 10:43, Laszlo Ersek wrote: >> On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: >>> 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é <philmd@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 <lersek@redhat.com> >>> +M: Philippe Mathieu-Daudé <philmd@redhat.com> >>> +S: Maintained >>> +F: docs/interop/firmware.json >>> +F: hw/firmware/uefi_edk2_crypto_policies.c >>> +F: include/hw/firmware/uefi_edk2.h >>> + >> >> I'm not happy with this. >> >> First, "docs/interop/firmware.json" is meant for more than just EDK2. We >> shouldn't list it in a section called "EDK2 Firmware". I can't suggest >> an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one >> would be misleading. >> >> Second, we expose fw_cfg files to edk2 platform firmware from a bunch of >> other places. For example -- and in this case I do mean to provide a >> complex example! --, see "etc/smi/supported-features", >> "etc/smi/requested-features", and "etc/smi/features-ok", in file >> "hw/isa/lpc_ich9.c". I'm unconvinced that the present feature merits new >> directories and new files. I'm not sure you say this is an example to follow or avoid... So I understand the EDK2 crypto policies paths shouldn't be default, only added on user/management request. Currently these can't be added if there is no such 'https' object. >> >> Then again, I also don't know where to put the logic. I guess I'll have >> to defer to more experienced reviewers. >> >> [snipping lots of QOM boilerplate] >> >>> +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(). >>> + * 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). >>> + */ >>> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", >>> + s->cacerts_path, NULL); >>> + } >>> +} >> >> Shouldn't we do some error checking here? >> >> I mean, printing an error message in fw_cfg_add_file_from_host(), and >> then continuing without exposing the named files in question to the >> firmware, could be OK if this was a "default on" feature. But (IIUC) >> here the user provided an explicit "-object" option, and we've just >> failed to construct the object. Doesn't such a situation usually prevent >> QEMU startup? Yes, it is fixed in v4. > Wait, I could be totally confused here. (Returning to this patch after > seeing the rest of the series.) > > Is it actually the case that the Edk2Crypto object holds nothing more > than two pathnames -- and so its construction can virtually never fail? > While the actual fw_cfg population occurs separately, in a machine_done > notifier? > > If that's the case, I don't think it's the right approach. Reading the > host files, and populating fw_cfg with them, should be part of the > object construction. And if those steps fail, the object should not be > possible to construct. > > We did something similar with the vmgenid device [hw/acpi/vmgenid.c], if > I remember correctly. It also has a dependency on fw_cfg... > > Ahh, no, I'm absolutely wrong about that. vmgenid_realize() doesn't do > anything with fw_cfg. Instead, we have acpi_setup() in > "hw/i386/acpi-build.c", which calls find_vmgenid_dev() and > vmgenid_add_fw_cfg(). And acpi_setup() is certainly called from > pc_machine_done(). > > In other words, the pattern that you use here matches existing practice. > Realize the device (or object) first, then add the fw_cfg thingies in > the "machine done" callback. OK. > > *Still*, I would like to see better error handling/reporting (or an > explanation why I'm wrong). How about reworking the edk2crypto class > itself -- it shouldn't just hold the pathnames of the files, but also > their contents. That is: > > - g_file_get_contents() should be called in the realize method > - the object would own the file contents > - the realize method would ensure that there wouldn't be any other > instance of the class (i.e. that it would be a singleton -- see the same > idea in vmgenid) > - there would be no need for the fw_cfg_add_file_from_host() API > - the machine done notifier would be extended to locate the object > (there could be zero or one instances), and if the one instance were > found, the machine done callback would hook the file contents into > fw_cfg. fw_cfg_add_file() cannot fail, so no errors to report at this stage. OK, this looks like a cleaner design, thanks! > Again I think this would follow the pattern from vmgenid. > > Thanks, > Laszlo > > >>> 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é <philmd@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); >>> + >>> +#endif /* HW_FIRMWARE_UEFI_EDK2_H */ >>> >> >
Hi Laszlo, On 3/13/19 11:11 AM, Laszlo Ersek wrote: > On 03/13/19 10:43, Laszlo Ersek wrote: >> On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: >>> 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é <philmd@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 <lersek@redhat.com> >>> +M: Philippe Mathieu-Daudé <philmd@redhat.com> >>> +S: Maintained >>> +F: docs/interop/firmware.json >>> +F: hw/firmware/uefi_edk2_crypto_policies.c >>> +F: include/hw/firmware/uefi_edk2.h >>> + >> >> I'm not happy with this. >> >> First, "docs/interop/firmware.json" is meant for more than just EDK2. We >> shouldn't list it in a section called "EDK2 Firmware". I can't suggest >> an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one >> would be misleading. >> >> Second, we expose fw_cfg files to edk2 platform firmware from a bunch of >> other places. For example -- and in this case I do mean to provide a >> complex example! --, see "etc/smi/supported-features", >> "etc/smi/requested-features", and "etc/smi/features-ok", in file >> "hw/isa/lpc_ich9.c". I'm unconvinced that the present feature merits new >> directories and new files. >> >> Then again, I also don't know where to put the logic. I guess I'll have >> to defer to more experienced reviewers. >> >> [snipping lots of QOM boilerplate] >> >>> +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(). >>> + * 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). >>> + */ >>> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", >>> + s->cacerts_path, NULL); >>> + } >>> +} >> >> Shouldn't we do some error checking here? >> >> I mean, printing an error message in fw_cfg_add_file_from_host(), and >> then continuing without exposing the named files in question to the >> firmware, could be OK if this was a "default on" feature. But (IIUC) >> here the user provided an explicit "-object" option, and we've just >> failed to construct the object. Doesn't such a situation usually prevent >> QEMU startup? > > Wait, I could be totally confused here. (Returning to this patch after > seeing the rest of the series.) > > Is it actually the case that the Edk2Crypto object holds nothing more > than two pathnames -- and so its construction can virtually never fail? > While the actual fw_cfg population occurs separately, in a machine_done > notifier? > > If that's the case, I don't think it's the right approach. Reading the > host files, and populating fw_cfg with them, should be part of the > object construction. And if those steps fail, the object should not be > possible to construct. > > We did something similar with the vmgenid device [hw/acpi/vmgenid.c], if > I remember correctly. It also has a dependency on fw_cfg... > > Ahh, no, I'm absolutely wrong about that. vmgenid_realize() doesn't do > anything with fw_cfg. Instead, we have acpi_setup() in > "hw/i386/acpi-build.c", which calls find_vmgenid_dev() and > vmgenid_add_fw_cfg(). And acpi_setup() is certainly called from > pc_machine_done(). > > In other words, the pattern that you use here matches existing practice. > Realize the device (or object) first, then add the fw_cfg thingies in > the "machine done" callback. OK. > > *Still*, I would like to see better error handling/reporting (or an > explanation why I'm wrong). How about reworking the edk2crypto class > itself -- it shouldn't just hold the pathnames of the files, but also > their contents. That is: > > - g_file_get_contents() should be called in the realize method > - the object would own the file contents > - the realize method would ensure that there wouldn't be any other > instance of the class (i.e. that it would be a singleton -- see the same > idea in vmgenid) > - there would be no need for the fw_cfg_add_file_from_host() API > - the machine done notifier would be extended to locate the object > (there could be zero or one instances), and if the one instance were > found, the machine done callback would hook the file contents into > fw_cfg. fw_cfg_add_file() cannot fail, so no errors to report at this stage. > > Again I think this would follow the pattern from vmgenid. I want to say I am impressed by your deep review. Your design is obviously way cleaner/safer. I think I was missing some part of the big picture here, thank you for your detailed comments! I did not know how vmgenid is processed. The only difference is I don't want the edk2crypto class to be a device, but rather a simple user object, and we already have an interface that does that: TYPE_USER_CREATABLE. Its UserCreatableClass::complete() method is similar to DeviceClass::realize() in managing errors at object instantiation, so the machine done notifier never fails. I'll respin. Regards, Phil.
On 06/20/19 14:07, Philippe Mathieu-Daudé wrote: > Hi Laszlo, > > On 3/13/19 11:11 AM, Laszlo Ersek wrote: >> On 03/13/19 10:43, Laszlo Ersek wrote: >>> On 03/10/19 01:47, Philippe Mathieu-Daudé wrote: >>>> 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é <philmd@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 <lersek@redhat.com> >>>> +M: Philippe Mathieu-Daudé <philmd@redhat.com> >>>> +S: Maintained >>>> +F: docs/interop/firmware.json >>>> +F: hw/firmware/uefi_edk2_crypto_policies.c >>>> +F: include/hw/firmware/uefi_edk2.h >>>> + >>> >>> I'm not happy with this. >>> >>> First, "docs/interop/firmware.json" is meant for more than just EDK2. We >>> shouldn't list it in a section called "EDK2 Firmware". I can't suggest >>> an alternative (MAINTAINERS is *huge* -- 2500+ lines), but this one >>> would be misleading. >>> >>> Second, we expose fw_cfg files to edk2 platform firmware from a bunch of >>> other places. For example -- and in this case I do mean to provide a >>> complex example! --, see "etc/smi/supported-features", >>> "etc/smi/requested-features", and "etc/smi/features-ok", in file >>> "hw/isa/lpc_ich9.c". I'm unconvinced that the present feature merits new >>> directories and new files. >>> >>> Then again, I also don't know where to put the logic. I guess I'll have >>> to defer to more experienced reviewers. >>> >>> [snipping lots of QOM boilerplate] >>> >>>> +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(). >>>> + * 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). >>>> + */ >>>> + fw_cfg_add_file_from_host(fw_cfg, "etc/edk2/https/cacerts", >>>> + s->cacerts_path, NULL); >>>> + } >>>> +} >>> >>> Shouldn't we do some error checking here? >>> >>> I mean, printing an error message in fw_cfg_add_file_from_host(), and >>> then continuing without exposing the named files in question to the >>> firmware, could be OK if this was a "default on" feature. But (IIUC) >>> here the user provided an explicit "-object" option, and we've just >>> failed to construct the object. Doesn't such a situation usually prevent >>> QEMU startup? >> >> Wait, I could be totally confused here. (Returning to this patch after >> seeing the rest of the series.) >> >> Is it actually the case that the Edk2Crypto object holds nothing more >> than two pathnames -- and so its construction can virtually never fail? >> While the actual fw_cfg population occurs separately, in a machine_done >> notifier? >> >> If that's the case, I don't think it's the right approach. Reading the >> host files, and populating fw_cfg with them, should be part of the >> object construction. And if those steps fail, the object should not be >> possible to construct. >> >> We did something similar with the vmgenid device [hw/acpi/vmgenid.c], if >> I remember correctly. It also has a dependency on fw_cfg... >> >> Ahh, no, I'm absolutely wrong about that. vmgenid_realize() doesn't do >> anything with fw_cfg. Instead, we have acpi_setup() in >> "hw/i386/acpi-build.c", which calls find_vmgenid_dev() and >> vmgenid_add_fw_cfg(). And acpi_setup() is certainly called from >> pc_machine_done(). >> >> In other words, the pattern that you use here matches existing practice. >> Realize the device (or object) first, then add the fw_cfg thingies in >> the "machine done" callback. OK. >> >> *Still*, I would like to see better error handling/reporting (or an >> explanation why I'm wrong). How about reworking the edk2crypto class >> itself -- it shouldn't just hold the pathnames of the files, but also >> their contents. That is: >> >> - g_file_get_contents() should be called in the realize method >> - the object would own the file contents >> - the realize method would ensure that there wouldn't be any other >> instance of the class (i.e. that it would be a singleton -- see the same >> idea in vmgenid) >> - there would be no need for the fw_cfg_add_file_from_host() API >> - the machine done notifier would be extended to locate the object >> (there could be zero or one instances), and if the one instance were >> found, the machine done callback would hook the file contents into >> fw_cfg. fw_cfg_add_file() cannot fail, so no errors to report at this stage. >> >> Again I think this would follow the pattern from vmgenid. > > I want to say I am impressed by your deep review. Your design is > obviously way cleaner/safer. I think I was missing some part of the big > picture here, thank you for your detailed comments! > > I did not know how vmgenid is processed. The only difference is I don't > want the edk2crypto class to be a device, but rather a simple user > object, and we already have an interface that does that: > TYPE_USER_CREATABLE. Its UserCreatableClass::complete() method is > similar to DeviceClass::realize() in managing errors at object > instantiation, so the machine done notifier never fails. > I'll respin. Sounds good to me, thanks! Please do CC reviewers with QOM experience, as I'm not familiar with TYPE_USER_CREATABLE. Thanks! Laszlo
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 <lersek@redhat.com> +M: Philippe Mathieu-Daudé <philmd@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é <philmd@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) + +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; + +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(). + * 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). + */ + 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é <philmd@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); + +#endif /* HW_FIRMWARE_UEFI_EDK2_H */
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é <philmd@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