[Qemu-devel] [PATCH v3 0/4] fw_cfg: Add edk2_add_host_crypto_policy()

[Qemu-devel] [PATCH v3 0/4] fw_cfg: Add edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Hi,

This series consists of:
- add fw_cfg_add_file_from_host()
- add edk2_add_host_crypto_policy() and the Edk2Crypto object

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

Since v2:
- Split of
Since v1:
- Addressed Michael and Laszlo comments.

Please review,

Phil.

v2: https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg02522.html
v1: https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg01598.html

Philippe Mathieu-Daudé (4):
  hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()
  hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()
  hw/i386: Use edk2_add_host_crypto_policy()
  hw/arm/virt: Use edk2_add_host_crypto_policy()

 MAINTAINERS                             |   8 ++
 hw/Makefile.objs                        |   1 +
 hw/arm/virt.c                           |   7 +
 hw/firmware/Makefile.objs               |   1 +
 hw/firmware/uefi_edk2_crypto_policies.c | 177 ++++++++++++++++++++++++
 hw/i386/pc.c                            |   7 +
 hw/nvram/fw_cfg.c                       |  21 +++
 include/hw/firmware/uefi_edk2.h         |  28 ++++
 include/hw/nvram/fw_cfg.h               |  23 +++
 9 files changed, 273 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

-- 
2.20.1

[Qemu-devel] [PATCH v3 1/4] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Philippe Mathieu-Daudé]

Add a function to read the full content of file on the host, and add
a new 'file' name item to the fw_cfg device.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v2: s/ptr/data, corrected documentation (Laszlo)
v3: inverted the if() logic
---
 hw/nvram/fw_cfg.c         | 21 +++++++++++++++++++++
 include/hw/nvram/fw_cfg.h | 23 +++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 7fdf04adc9..2a345bfd5c 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -826,6 +826,27 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
     fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
 }
 
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len)
+{
+    GError *gerr = NULL;
+    gchar *data = NULL;
+    gsize contents_len = 0;
+
+    if (!g_file_get_contents(host_path, &data, &contents_len, &gerr)) {
+        error_report("%s", gerr->message);
+        g_error_free(gerr);
+        return NULL;
+    }
+    fw_cfg_add_file(s, filename, data, contents_len);
+
+    if (len) {
+        *len = contents_len;
+    }
+
+    return data;
+}
+
 void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
                         void *data, size_t len)
 {
diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
index f5a6895a74..7c4dbe2a2a 100644
--- a/include/hw/nvram/fw_cfg.h
+++ b/include/hw/nvram/fw_cfg.h
@@ -166,6 +166,29 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
 void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
                      size_t len);
 
+/**
+ * fw_cfg_add_file_from_host:
+ * @s: fw_cfg device being modified
+ * @filename: name of new fw_cfg file item
+ * @host_path: path of the host file to read the data from
+ * @len: pointer to hold the length of the host file (optional)
+ *
+ * Read the content of a host file as a raw "blob" then add a new NAMED
+ * fw_cfg item of the file size. If @len is provided, it will contain the
+ * total length read from the host file. The data read from the host
+ * filesystem is owned by the new fw_cfg entry, and is stored into the data
+ * structure of the fw_cfg device.
+ * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
+ * will be used; also, a new entry will be added to the file directory
+ * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
+ * data size, and assigned selector key value.
+ *
+ * Returns: pointer to the newly allocated file content, or NULL if an error
+ * occured. The returned pointer must be freed with g_free().
+ */
+void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
+                                const char *host_path, size_t *len);
+
 /**
  * fw_cfg_add_file_callback:
  * @s: fw_cfg device being modified
-- 
2.20.1

[Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

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)
+
+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 */
-- 
2.20.1

[Qemu-devel] [PATCH v3 3/4] hw/i386: Use edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Enable the EDK2 Crypto Policy features on the PC machine.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/i386/pc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/i386/pc.c b/hw/i386/pc.c
index 42128183e9..00dc377df0 100644
--- a/hw/i386/pc.c
+++ b/hw/i386/pc.c
@@ -38,6 +38,7 @@
 #include "hw/nvram/fw_cfg.h"
 #include "hw/timer/hpet.h"
 #include "hw/firmware/smbios.h"
+#include "hw/firmware/uefi_edk2.h"
 #include "hw/loader.h"
 #include "elf.h"
 #include "multiboot.h"
@@ -1046,6 +1047,11 @@ static FWCfgState *bochs_bios_init(AddressSpace *as, PCMachineState *pcms)
     return fw_cfg;
 }
 
+static void pc_uefi_setup(PCMachineState *pcms)
+{
+    edk2_add_host_crypto_policy(pcms->fw_cfg);
+}
+
 static long get_file_size(FILE *f)
 {
     long where, size;
@@ -1645,6 +1651,7 @@ void pc_machine_done(Notifier *notifier, void *data)
     if (pcms->fw_cfg) {
         pc_build_smbios(pcms);
         pc_build_feature_control_file(pcms);
+        pc_uefi_setup(pcms);
         /* update FW_CFG_NB_CPUS to account for -device added CPUs */
         fw_cfg_modify_i16(pcms->fw_cfg, FW_CFG_NB_CPUS, pcms->boot_cpus);
     }
-- 
2.20.1

[Qemu-devel] [PATCH v3 4/4] hw/arm/virt: Use edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

Enable the EDK2 Crypto Policy features on the Virt machine.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/arm/virt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 7f66ddad89..940f41056a 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -55,6 +55,7 @@
 #include "hw/intc/arm_gicv3_common.h"
 #include "kvm_arm.h"
 #include "hw/firmware/smbios.h"
+#include "hw/firmware/uefi_edk2.h"
 #include "qapi/visitor.h"
 #include "standard-headers/linux/input.h"
 #include "hw/arm/smmuv3.h"
@@ -1304,6 +1305,11 @@ static void virt_build_smbios(VirtMachineState *vms)
     }
 }
 
+static void virt_uefi_setup(VirtMachineState *vms)
+{
+    edk2_add_host_crypto_policy(vms->fw_cfg);
+}
+
 static
 void virt_machine_done(Notifier *notifier, void *data)
 {
@@ -1332,6 +1338,7 @@ void virt_machine_done(Notifier *notifier, void *data)
 
     virt_acpi_setup(vms);
     virt_build_smbios(vms);
+    virt_uefi_setup(vms);
 }
 
 static uint64_t virt_cpu_mp_affinity(VirtMachineState *vms, int idx)
-- 
2.20.1

Re: [Qemu-devel] [PATCH v3 1/4] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host()

[Markus Armbruster]

Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> Add a function to read the full content of file on the host, and add
> a new 'file' name item to the fw_cfg device.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> v2: s/ptr/data, corrected documentation (Laszlo)
> v3: inverted the if() logic
> ---
>  hw/nvram/fw_cfg.c         | 21 +++++++++++++++++++++
>  include/hw/nvram/fw_cfg.h | 23 +++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 7fdf04adc9..2a345bfd5c 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -826,6 +826,27 @@ void fw_cfg_add_file(FWCfgState *s,  const char *filename,
>      fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
>  }
>  
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len)
> +{
> +    GError *gerr = NULL;
> +    gchar *data = NULL;
> +    gsize contents_len = 0;
> +
> +    if (!g_file_get_contents(host_path, &data, &contents_len, &gerr)) {
> +        error_report("%s", gerr->message);
> +        g_error_free(gerr);
> +        return NULL;
> +    }
> +    fw_cfg_add_file(s, filename, data, contents_len);
> +
> +    if (len) {
> +        *len = contents_len;
> +    }
> +
> +    return data;
> +}
> +
>  void *fw_cfg_modify_file(FWCfgState *s, const char *filename,
>                          void *data, size_t len)
>  {
> diff --git a/include/hw/nvram/fw_cfg.h b/include/hw/nvram/fw_cfg.h
> index f5a6895a74..7c4dbe2a2a 100644
> --- a/include/hw/nvram/fw_cfg.h
> +++ b/include/hw/nvram/fw_cfg.h
> @@ -166,6 +166,29 @@ void fw_cfg_add_i64(FWCfgState *s, uint16_t key, uint64_t value);
>  void fw_cfg_add_file(FWCfgState *s, const char *filename, void *data,
>                       size_t len);
>  
> +/**
> + * fw_cfg_add_file_from_host:
> + * @s: fw_cfg device being modified
> + * @filename: name of new fw_cfg file item
> + * @host_path: path of the host file to read the data from
> + * @len: pointer to hold the length of the host file (optional)
> + *
> + * Read the content of a host file as a raw "blob" then add a new NAMED
> + * fw_cfg item of the file size. If @len is provided, it will contain the
> + * total length read from the host file. The data read from the host
> + * filesystem is owned by the new fw_cfg entry, and is stored into the data
> + * structure of the fw_cfg device.
> + * The next available (unused) selector key starting at FW_CFG_FILE_FIRST
> + * will be used; also, a new entry will be added to the file directory
> + * structure residing at key value FW_CFG_FILE_DIR, containing the item name,
> + * data size, and assigned selector key value.
> + *
> + * Returns: pointer to the newly allocated file content, or NULL if an error
> + * occured. The returned pointer must be freed with g_free().

In theory.  In practice, your callers (in PATCH 2) ignore the value.

The file contents needs to live as long as FWCfgState (something the
function comments in this file neglect to spell out; doc fix patch would
be nice, not necessarily in this series).  An FWCfgState generally
belongs to a machine (see PATCH 3+4).

It looks like we destroy neither the machine (in a quick test,
machine_finalize() never gets called), nor its fw_cfg device (if I add
an instance_finalize() method to TYPE_FW_CFG_IO, it doesn't get called),
so both machine and its FWCfgState live forever.  There's no point in
time where the caller can obey the function comment's demand to free
without leaving dangling pointers in FWCfgState.

Pushing the responsibility to free the file contents on the caller is
not a nice interface anyway.  I feel fw_cfg should take over ownership.
In the current state of things, that's trivial: document it does, done.

If we ever get around to cleaning up machines and onboard devices
properly, then fw_cfg.c will have to grow a suitable instance_finalize()
method, even without your patch.  We lack such resource cleanup all over
the place, but feel free to a TODO comment here anyway.

Are there more functions in fw_cfg.h with the same interface issue?
Besides FWCfgState constructors, the only one returning a pointer is
fw_cfg_modify_file().  Function comment:

 * Replace a NAMED fw_cfg item. If an existing item is found, its callback
 * information will be cleared, and a pointer to its data will be returned
 * to the caller, so that it may be freed if necessary. If an existing item
 * is not found, this call defaults to fw_cfg_add_file(), and NULL is
 * returned to the caller.

Okay, not the same issue, but there's still an issue: this function's
caller needs to know how the old file contents was added!  Remember,
fw_cfg_add_file() lets you add both malloc'ed and other memory.  Needs
an interface fix or at least a comment pointing out the issue.  Separate
patch, not necessarily in this series.

> + */
> +void *fw_cfg_add_file_from_host(FWCfgState *s, const char *filename,
> +                                const char *host_path, size_t *len);
> +
>  /**
>   * fw_cfg_add_file_callback:
>   * @s: fw_cfg device being modified

Re: [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Markus Armbruster]

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

Re: [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

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

Re: [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Philippe Mathieu-Daudé]

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.

Re: [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

[Laszlo Ersek]

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