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

[Laszlo Ersek]

On 03/11/19 19:27, Philippe Mathieu-Daudé wrote:
> On 3/11/19 8:26 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <address@hidden> 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é <address@hidden>
>>> ---
>>> 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 <address@hidden>
>>> +M: Philippe Mathieu-Daudé <address@hidden>
>>> +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é <address@hidden>
>>> + *
>>> + * 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é <address@hidden>
>>> + *
>>> + * 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 */