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

Sounds good to me, thanks! Please do CC reviewers with QOM experience,
as I'm not familiar with TYPE_USER_CREATABLE.

Thanks!
Laszlo