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

[Laszlo Ersek]

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?

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 */
>