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é <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.

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.