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é <phi...@redhat.com> >>>> --- >>>> v3: >>>> - '-object' -> '--object' in commit description (Eric) >>>> - reworded the 'TODO: g_free' comment >>>> --- >>>> MAINTAINERS | 8 ++ >>>> hw/Makefile.objs | 1 + >>>> hw/firmware/Makefile.objs | 1 + >>>> hw/firmware/uefi_edk2_crypto_policies.c | 177 ++++++++++++++++++++++++ >>>> include/hw/firmware/uefi_edk2.h | 28 ++++ >>>> 5 files changed, 215 insertions(+) >>>> create mode 100644 hw/firmware/Makefile.objs >>>> create mode 100644 hw/firmware/uefi_edk2_crypto_policies.c >>>> create mode 100644 include/hw/firmware/uefi_edk2.h >>>> >>>> diff --git a/MAINTAINERS b/MAINTAINERS >>>> index cf09a4c127..70122b3d0d 100644 >>>> --- a/MAINTAINERS >>>> +++ b/MAINTAINERS >>>> @@ -2206,6 +2206,14 @@ F: include/hw/i2c/smbus_master.h >>>> F: include/hw/i2c/smbus_slave.h >>>> F: include/hw/i2c/smbus_eeprom.h >>>> >>>> +EDK2 Firmware >>>> +M: Laszlo Ersek <ler...@redhat.com> >>>> +M: Philippe Mathieu-Daudé <phi...@redhat.com> >>>> +S: Maintained >>>> +F: docs/interop/firmware.json >>>> +F: hw/firmware/uefi_edk2_crypto_policies.c >>>> +F: include/hw/firmware/uefi_edk2.h >>>> + >>> >>> 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