From: Laszlo Ersek <lersek@redhat.com>
To: "Philippe Mathieu-Daudé" <philmd@redhat.com>,
"Gerd Hoffmann" <kraxel@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
qemu-devel@nongnu.org
Cc: "Daniel P . Berrange" <berrange@redhat.com>,
Markus Armbruster <armbru@redhat.com>
Subject: Re: [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()
Date: Thu, 20 Jun 2019 15:51:50 +0200 [thread overview]
Message-ID: <c5fed053-5dcf-afef-010b-c261741c748f@redhat.com> (raw)
In-Reply-To: <88b7370c-1b6f-11e0-9fbe-d532e5726aa6@redhat.com>
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é <philmd@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 <lersek@redhat.com>
>>>> +M: Philippe Mathieu-Daudé <philmd@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
next prev parent reply other threads:[~2019-06-20 14:50 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-03-10 0:47 [Qemu-devel] [PATCH v3 0/4] fw_cfg: Add edk2_add_host_crypto_policy() Philippe Mathieu-Daudé
2019-03-10 0:47 ` [Qemu-devel] [PATCH v3 1/4] hw/nvram/fw_cfg: Add fw_cfg_add_file_from_host() Philippe Mathieu-Daudé
2019-03-11 7:15 ` Markus Armbruster
2019-03-10 0:47 ` [Qemu-devel] [PATCH v3 2/4] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy() Philippe Mathieu-Daudé
2019-03-11 7:26 ` Markus Armbruster
2019-03-11 18:27 ` Philippe Mathieu-Daudé
[not found] ` <5e2f3651-5f00-0c93-353e-e58f079998e9@redhat.com>
[not found] ` <124e54f9-c7e1-0157-61f1-673154872749@redhat.com>
2019-06-20 12:07 ` Philippe Mathieu-Daudé
2019-06-20 13:51 ` Laszlo Ersek [this message]
2019-03-10 0:47 ` [Qemu-devel] [PATCH v3 3/4] hw/i386: Use edk2_add_host_crypto_policy() Philippe Mathieu-Daudé
2019-03-10 0:47 ` [Qemu-devel] [PATCH v3 4/4] hw/arm/virt: " Philippe Mathieu-Daudé
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c5fed053-5dcf-afef-010b-c261741c748f@redhat.com \
--to=lersek@redhat.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=kraxel@redhat.com \
--cc=mst@redhat.com \
--cc=philmd@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).