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

[Laszlo Ersek]

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é <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
>> +
> 
> 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é <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);
>> +
>> +#endif /* HW_FIRMWARE_UEFI_EDK2_H */
>>
>