diff mbox series

[v5,1/3] hw/firmware: Add Edk2Crypto and edk2_add_host_crypto_policy()

Message ID 20190620122132.10075-2-philmd@redhat.com
State New
Headers show
Series fw_cfg: Add edk2_add_host_crypto_policy() | expand

Commit Message

Philippe Mathieu-Daudé June 20, 2019, 12:21 p.m. UTC
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:

- via the command line:

  $ 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

- via QMP:

  {
    "execute": "object-add",
    "arguments": {
      "qom-type": "edk2_crypto",
      "id": "https",
      "props": {
        "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:
- inverted the if() logic
- '-object' -> '--object' in commit description (Eric)
- reworded the 'TODO: g_free' comment
v4:
- do not return pointer to alloc'd data (Markus)
- INTERFACE_CHECK -> OBJECT_CLASS_CHECK (Markus)
- path -> filename (Laszlo)
- dropped the 'TODO: g_free' comment (Markus)
v5:
- only allow 1 singleton using the UserCreatableClass::complete
  callback (Markus, Laszlo)
- object own fw_cfg 'file' content, no need for
  fw_cfg_add_file_from_host() (Laszlo)
- g_file_get_contents() called when object is instantiated
  and report error, the machine 'done' notifier do not have
  to manage errors (do not fail).
- add QMP example
-
- do not add docs/interop/firmware.json to MAINTAINERS
---
 MAINTAINERS                             |   2 +
 hw/Makefile.objs                        |   1 +
 hw/firmware/Makefile.objs               |   1 +
 hw/firmware/uefi_edk2_crypto_policies.c | 209 ++++++++++++++++++++++++
 include/hw/firmware/uefi_edk2.h         |  30 ++++
 5 files changed, 243 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

Comments

Laszlo Ersek June 24, 2019, 2:53 p.m. UTC | #1
(+Daniel)

On 06/20/19 14:21, 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 [*].

(1) s/usercase/use case/, s/Boof/Boot/

> 
> Usage example:
> 
> - via the command line:
> 
>   $ 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
> 
> - via QMP:
> 
>   {
>     "execute": "object-add",
>     "arguments": {
>       "qom-type": "edk2_crypto",
>       "id": "https",
>       "props": {
>         "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:
> - inverted the if() logic
> - '-object' -> '--object' in commit description (Eric)
> - reworded the 'TODO: g_free' comment
> v4:
> - do not return pointer to alloc'd data (Markus)
> - INTERFACE_CHECK -> OBJECT_CLASS_CHECK (Markus)
> - path -> filename (Laszlo)
> - dropped the 'TODO: g_free' comment (Markus)
> v5:
> - only allow 1 singleton using the UserCreatableClass::complete
>   callback (Markus, Laszlo)
> - object own fw_cfg 'file' content, no need for
>   fw_cfg_add_file_from_host() (Laszlo)
> - g_file_get_contents() called when object is instantiated
>   and report error, the machine 'done' notifier do not have
>   to manage errors (do not fail).
> - add QMP example
> -
> - do not add docs/interop/firmware.json to MAINTAINERS
> ---
>  MAINTAINERS                             |   2 +
>  hw/Makefile.objs                        |   1 +
>  hw/firmware/Makefile.objs               |   1 +
>  hw/firmware/uefi_edk2_crypto_policies.c | 209 ++++++++++++++++++++++++
>  include/hw/firmware/uefi_edk2.h         |  30 ++++
>  5 files changed, 243 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 d32c5c2313..28de489134 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2262,6 +2262,8 @@ EDK2 Firmware
>  M: Laszlo Ersek <lersek@redhat.com>
>  M: Philippe Mathieu-Daudé <philmd@redhat.com>
>  S: Supported
> +F: hw/firmware/uefi_edk2_crypto_policies.c
> +F: include/hw/firmware/uefi_edk2.h
>  F: pc-bios/descriptors/??-edk2-*.json
>  F: pc-bios/edk2-*
>  F: roms/Makefile.edk2
> diff --git a/hw/Makefile.objs b/hw/Makefile.objs
> index d770926ba9..c13b6ee0dd 100644
> --- a/hw/Makefile.objs
> +++ b/hw/Makefile.objs
> @@ -8,6 +8,7 @@ devices-dirs-$(CONFIG_SOFTMMU) += char/
>  devices-dirs-$(CONFIG_SOFTMMU) += cpu/
>  devices-dirs-$(CONFIG_SOFTMMU) += display/
>  devices-dirs-$(CONFIG_SOFTMMU) += dma/
> +devices-dirs-$(CONFIG_SOFTMMU) += firmware/
>  devices-dirs-$(CONFIG_SOFTMMU) += gpio/
>  devices-dirs-$(CONFIG_HYPERV) += hyperv/
>  devices-dirs-$(CONFIG_I2C) += i2c/

(1) I wonder if this granularity is the right one. Do we really want to
link this code into all softmmu targets, even if they (a) don't support
fw_cfg, (b) and/or have no bindings (per spec) for UEFI?

I can't recommend anything better (i.e. I can't really be "constructive"
here), but the above seems a bit to broad. In practice anyway, we only
need this for the i386, x86_64, arm, and aarch64 softmmu targets (for
now anyway).

FWIW, the vmgenid device, which we tend to use as an example here (for
some aspects anyway), seems to have its own knob: CONFIG_ACPI_VMGENID.


> diff --git a/hw/firmware/Makefile.objs b/hw/firmware/Makefile.objs
> new file mode 100644
> index 0000000000..ea1f6d44df
> --- /dev/null
> +++ b/hw/firmware/Makefile.objs
> @@ -0,0 +1 @@
> +common-obj-y += uefi_edk2_crypto_policies.o
> diff --git a/hw/firmware/uefi_edk2_crypto_policies.c b/hw/firmware/uefi_edk2_crypto_policies.c
> new file mode 100644
> index 0000000000..a0164272ea
> --- /dev/null
> +++ b/hw/firmware/uefi_edk2_crypto_policies.c
> @@ -0,0 +1,209 @@
> +/*
> + * UEFI EDK2 Support
> + *
> + * Copyright (c) 2019 Red Hat Inc.
> + *
> + * Author:
> + *  Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +
> +#include "qemu/osdep.h"
> +#include "qapi/error.h"
> +#include "qom/object_interfaces.h"
> +#include "hw/firmware/uefi_edk2.h"
> +
> +
> +#define TYPE_EDK2_CRYPTO "edk2_crypto"
> +
> +#define EDK2_CRYPTO_CLASS(klass) \
> +     OBJECT_CLASS_CHECK(Edk2CryptoClass, (klass), \
> +                        TYPE_EDK2_CRYPTO)
> +#define EDK2_CRYPTO_GET_CLASS(obj) \
> +     OBJECT_GET_CLASS(Edk2CryptoClass, (obj), \
> +                      TYPE_EDK2_CRYPTO)
> +#define EDK2_CRYPTO(obj) \
> +     OBJECT_CHECK(Edk2Crypto, (obj), \
> +                  TYPE_EDK2_CRYPTO)
> +
> +typedef struct FWCfgHostContent {
> +    /*
> +     * Path to the acceptable ciphersuites and the preferred order from
> +     * the host-side crypto policy.
> +     */
> +    char *filename;

(2) The leading comment on this member is too specific. It is correct
for the "Edk2Crypto.ciphers" member (below) only.

How about simply stating: "load @contents from the host-side file
identified by @filename".

> +    /*
> +     * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
> +     * referenced by the starting pointer is only linked, NOT copied, into the
> +     * data structure of the fw_cfg device.
> +     */
> +    char *contents;
> +
> +    size_t contents_length;
> +} FWCfgHostContent;
> +
> +typedef struct Edk2Crypto {
> +    Object parent_obj;
> +
> +    /*
> +     * Path to the acceptable ciphersuites and the preferred order from
> +     * the host-side crypto policy.
> +     */
> +    FWCfgHostContent ciphers;
> +    /* Path to the trusted CA certificates configured on the host side. */
> +    FWCfgHostContent cacerts;
> +} Edk2Crypto;

(3) This looks good to me, but I'd like to request some more documentation.

The commit message already references
<https://github.com/tianocore/edk2/blob/master/OvmfPkg/README>, and
that's good. However:

(3a) I'd like to see that OvmfPkg/README reference either moved here, or
duplicated here (right after "parent_obj")

(3b) just above "ciphers', please reference (in addition to the current
comment):

  https://gitlab.com/redhat-crypto/fedora-crypto-policies/issues/12

(3c) just above "cacerts", please reference (in addition to the current
comment):

  p11-kit commit ee27f9153a14 (trust: implement the "edk2-cacerts"
extractor)

> +
> +typedef struct Edk2CryptoClass {
> +    ObjectClass parent_class;
> +} Edk2CryptoClass;
> +
> +static Edk2Crypto *edk2_crypto_by_policy_id(const char *policy_id, Error **errp)
> +{
> +    Object *obj;
> +
> +    obj = object_resolve_path_component(object_get_objects_root(), policy_id);
> +    if (!obj) {
> +        error_setg(errp, "Cannot find EDK2 crypto policy ID %s", policy_id);
> +        return NULL;
> +    }
> +
> +    if (!object_dynamic_cast(obj, TYPE_EDK2_CRYPTO)) {
> +        error_setg(errp, "Object '%s' is not a EDK2 crypto subclass",
> +                         policy_id);

(4) The indentation looks off.

> +        return NULL;
> +    }
> +
> +    return EDK2_CRYPTO(obj);
> +}
> +
> +static void edk2_crypto_prop_set_ciphers(Object *obj, const char *value,
> +                                         Error **errp G_GNUC_UNUSED)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(obj);
> +
> +    g_free(s->ciphers.filename);
> +    s->ciphers.filename = g_strdup(value);
> +}
> +
> +static char *edk2_crypto_prop_get_ciphers(Object *obj,
> +                                          Error **errp G_GNUC_UNUSED)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(obj);
> +
> +    return g_strdup(s->ciphers.filename);
> +}
> +
> +static void edk2_crypto_prop_set_cacerts(Object *obj, const char *value,
> +                                         Error **errp G_GNUC_UNUSED)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(obj);
> +
> +    g_free(s->cacerts.filename);
> +    s->cacerts.filename = g_strdup(value);
> +}
> +
> +static char *edk2_crypto_prop_get_cacerts(Object *obj,
> +                                          Error **errp G_GNUC_UNUSED)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(obj);
> +
> +    return g_strdup(s->cacerts.filename);
> +}
> +
> +static void edk2_crypto_complete(UserCreatable *uc, Error **errp)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(uc);
> +    Error *local_err = NULL;
> +    GError *gerr = NULL;
> +
> +    if (s->ciphers.filename) {
> +        if (!g_file_get_contents(s->ciphers.filename, &s->ciphers.contents,
> +                                 &s->ciphers.contents_length, &gerr)) {
> +            goto report_error;
> +        }
> +    }
> +    if (s->cacerts.filename) {
> +        if (!g_file_get_contents(s->cacerts.filename, &s->cacerts.contents,
> +                                 &s->cacerts.contents_length, &gerr)) {
> +            goto report_error;
> +        }
> +    }
> +    return;
> +
> + report_error:
> +    error_setg(&local_err, "%s", gerr->message);
> +    g_error_free(gerr);
> +    error_propagate_prepend(errp, local_err, "EDK2 crypto policy: ");
> +}

(5) Assuming we load the ciphers successfully, but fail to load the
cacerts, will this not leak the former?

(Well I see that the finalize method releases everything that's not
NULL, yet it feels strange to leave this function with a
half-constructed object, where "ciphers.filename" is not in sync with
"ciphers.contents".)


(6) The error_propagate_prepend() function must have been added since I
last looked :) , and the docs seem clear on it in "include/qapi/error.h".

Given that we construct the QAPI error here in the first place, can we
simplify the code as follows?

  error_setg(errp, "EDK2 crypto policy: %s", gerr->message);
  g_error_free(gerr);


> +
> +static void edk2_crypto_finalize(Object *obj)
> +{
> +    Edk2Crypto *s = EDK2_CRYPTO(obj);
> +
> +    g_free(s->ciphers.filename);
> +    g_free(s->ciphers.contents);
> +    g_free(s->cacerts.filename);
> +    g_free(s->cacerts.contents);
> +}
> +
> +static void edk2_crypto_class_init(ObjectClass *oc, void *data)
> +{
> +    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
> +
> +    ucc->complete = edk2_crypto_complete;
> +
> +    object_class_property_add_str(oc, "ciphers",
> +                                  edk2_crypto_prop_get_ciphers,
> +                                  edk2_crypto_prop_set_ciphers,
> +                                  NULL);
> +    object_class_property_add_str(oc, "cacerts",
> +                                  edk2_crypto_prop_get_cacerts,
> +                                  edk2_crypto_prop_set_cacerts,
> +                                  NULL);
> +}
> +
> +static const TypeInfo edk2_crypto_info = {
> +    .parent = TYPE_OBJECT,
> +    .name = TYPE_EDK2_CRYPTO,
> +    .instance_size = sizeof(Edk2Crypto),
> +    .instance_finalize = edk2_crypto_finalize,
> +    .class_size = sizeof(Edk2CryptoClass),
> +    .class_init = edk2_crypto_class_init,
> +    .interfaces = (InterfaceInfo[]) {
> +        { TYPE_USER_CREATABLE },
> +        { }
> +    }
> +};
> +
> +static void edk2_crypto_register_types(void)
> +{
> +    type_register_static(&edk2_crypto_info);
> +}
> +
> +type_init(edk2_crypto_register_types);
> +
> +static void edk2_add_host_crypto_policy_https(FWCfgState *fw_cfg)
> +{
> +    Edk2Crypto *s;
> +
> +    s = edk2_crypto_by_policy_id("https", NULL);
> +    if (!s) {
> +        return;
> +    }
> +    if (s->ciphers.contents_length) {
> +        fw_cfg_add_file(fw_cfg, "etc/edk2/https/ciphers",
> +                        s->ciphers.contents, s->ciphers.contents_length);
> +    }
> +    if (s->cacerts.contents_length) {
> +        fw_cfg_add_file(fw_cfg, "etc/edk2/https/cacerts",
> +                        s->cacerts.contents, s->cacerts.contents_length);
> +    }
> +}
> +
> +void edk2_add_host_crypto_policy(FWCfgState *fw_cfg)
> +{
> +    edk2_add_host_crypto_policy_https(fw_cfg);
> +}

(7) How is it ensured by this implementation that "id=https" is
specified at most twice, for this object type?

Is that guaranteed by general infrastructure?

I vaguely recall object_resolve_path_component() or similar failing if
there isn't exactly one match. But, you mention
"UserCreatableClass::complete" in the patch notes anyway, so the
conflict is likely detected even earlier.


> diff --git a/include/hw/firmware/uefi_edk2.h b/include/hw/firmware/uefi_edk2.h
> new file mode 100644
> index 0000000000..f8f81c5cb2
> --- /dev/null
> +++ b/include/hw/firmware/uefi_edk2.h
> @@ -0,0 +1,30 @@
> +/*
> + * UEFI EDK2 Support
> + *
> + * Copyright (c) 2019 Red Hat Inc.
> + *
> + * Author:
> + *  Philippe Mathieu-Daudé <philmd@redhat.com>
> + *
> + * 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:
> + * @fw_cfg: fw_cfg device being modified
> + *
> + * Add a new named file containing the host crypto policy.

(8) I'd use plural here ("add named files...")


> + *
> + * This method is called by the machine_done() Notifier of
> + * some implementations of MachineState, currently the X86
> + * PCMachineState and the ARM VirtMachineState.
> + */

(9) I suggest dropping the phrase starting with "currently".

The first half of the sentence is helpful, because it states a goal.

The second half of the sentence is bound to get stale at some point, and
whoever cares can run "git grep -w edk2_add_host_crypto_policy" at any time.

> +void edk2_add_host_crypto_policy(FWCfgState *fw_cfg);
> +
> +#endif /* HW_FIRMWARE_UEFI_EDK2_H */
> 

Thanks!
Laszlo
Daniel P. Berrangé June 24, 2019, 3:11 p.m. UTC | #2
On Thu, Jun 20, 2019 at 02:21:30PM +0200, 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:
> 
> - via the command line:
> 
>   $ 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
> 
> - via QMP:
> 
>   {
>     "execute": "object-add",
>     "arguments": {
>       "qom-type": "edk2_crypto",
>       "id": "https",
>       "props": {
>         "ciphers": "/etc/crypto-policies/back-ends/openssl.config",
>         "cacerts": "/etc/pki/ca-trust/extracted/edk2/cacerts.bin"
>       }
>     }
>   }

These commands / args create an object with ID "https" but you are
not telling anything to use this object, which makes me fear that
you have some code somewhere hardcoded to mandate an object with
an ID of "https".....


> +static void edk2_add_host_crypto_policy_https(FWCfgState *fw_cfg)
> +{
> +    Edk2Crypto *s;
> +
> +    s = edk2_crypto_by_policy_id("https", NULL);

....indeed you have hardcoded use of a particular object ID.

This is not good - choice of what strings to use for object IDs is something
for the the management app - QEMU must not be dictating certain magic IDs.

There needs to be a command line arg somewhere to tell the firmware what
object ID provides the data.

Given that you're triggering load of this object from the machine type
initializer code, having an arg to the -machine option is a natural
choice.

ie something like this:

   $ qemu-system-x86_64 \
       --object edk2_crypto,id=edkpolicy0,\
               ciphers=/etc/crypto-policies/back-ends/openssl.config,\
               cacerts=/etc/pki/ca-trust/extracted/edk2/cacerts.bin \
       --machine q35,edk2_crypto_policy=edkpolicy0

I don't see a compelling reason to separate https policy out as an
explicit tunable, distinct from other, yet to be invented, needs for
crypto policy even if EDK2 itself is fine grained in this way.

Regards,
Daniel
Laszlo Ersek June 24, 2019, 3:14 p.m. UTC | #3
On 06/24/19 16:53, Laszlo Ersek wrote:
> (+Daniel)
> 
> On 06/20/19 14:21, Philippe Mathieu-Daudé wrote:

>>   $ 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

(12) Regarding the command line. It just occurs to me that Daniel
suggested [*] that libvirt should not be taught about this feature
specifically.

Thus, I think we need properties that are "smarter" than plain
user-specified strings:

- they should have default values (the ones your example includes above)

- for each of the properties: if the default pathname fails to identify
a file, then treat it as a normal situation (leave the corresponding
fields NULL)

- if the user overrides the default, and the pathname resolution fails,
then that should generate an error

- the user should be permitted to override the default such that the
corresponding setting is disabled (i.e. no error, but also no loading)


It's too bad that I'm not sure about the right way to implement this. It
reminds me of On/Off/Auto, but only vaguely.

In fact, if we never want to teach libvirt about this feature, then we
essentially expect QEMU to auto-load these files, whenever they exist --
even if the guest ends up booting something other than edk2 firmware!

[*] https://bugzilla.redhat.com/show_bug.cgi?id=1536624#c11 --
unfortunately, this is a private RHBZ :(

Thanks
Laszlo
Daniel P. Berrangé June 24, 2019, 3:23 p.m. UTC | #4
On Mon, Jun 24, 2019 at 05:14:23PM +0200, Laszlo Ersek wrote:
> On 06/24/19 16:53, Laszlo Ersek wrote:
> > (+Daniel)
> > 
> > On 06/20/19 14:21, Philippe Mathieu-Daudé wrote:
> 
> >>   $ 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
> 
> (12) Regarding the command line. It just occurs to me that Daniel
> suggested [*] that libvirt should not be taught about this feature
> specifically.

Oh yes, thank you for reminding me of this point.

Essentially this is the same use case as with the GNUTLS default crypto
policy in QEMU.

The tls-creds-x409 object type in QEMU has a "priority" property which
lets you set a GNUTLS priority string eg

    --object tls-creds-x509,id=tls0,priority=@SYSTEM

In practice, however, libvirt never uses this because the QEMU configure
script has a --tls-priority arg and distros will use that arg to set the
desired priority at build time. eg Fedora/RHEL build QEMU with the
"--tls-priority=@QEMU,SYSTEM" string and libvirt hasn't bothered to
make use of the 'priority' arg for tls-creds-x509 as a result.

I'd expect similar to be done with EDK2 priority where Fedora/RHEL sets
a compile time ciphers/cacerts list in RPM build.

Contrary to what we did with GNUTLS though, libvirt might still allow
this to be overridden at runtime, since the handling of the ciphers
is not as smart as we get with GNUTLS priority setting.

> Thus, I think we need properties that are "smarter" than plain
> user-specified strings:
> 
> - they should have default values (the ones your example includes above)
> 
> - for each of the properties: if the default pathname fails to identify
> a file, then treat it as a normal situation (leave the corresponding
> fields NULL)
> 
> - if the user overrides the default, and the pathname resolution fails,
> then that should generate an error
> 
> - the user should be permitted to override the default such that the
> corresponding setting is disabled (i.e. no error, but also no loading)
> 
> 
> It's too bad that I'm not sure about the right way to implement this. It
> reminds me of On/Off/Auto, but only vaguely.
> 
> In fact, if we never want to teach libvirt about this feature, then we
> essentially expect QEMU to auto-load these files, whenever they exist --
> even if the guest ends up booting something other than edk2 firmware!
> 
> [*] https://bugzilla.redhat.com/show_bug.cgi?id=1536624#c11 --
> unfortunately, this is a private RHBZ :(
> 
> Thanks
> Laszlo

Regards,
Daniel
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index d32c5c2313..28de489134 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2262,6 +2262,8 @@  EDK2 Firmware
 M: Laszlo Ersek <lersek@redhat.com>
 M: Philippe Mathieu-Daudé <philmd@redhat.com>
 S: Supported
+F: hw/firmware/uefi_edk2_crypto_policies.c
+F: include/hw/firmware/uefi_edk2.h
 F: pc-bios/descriptors/??-edk2-*.json
 F: pc-bios/edk2-*
 F: roms/Makefile.edk2
diff --git a/hw/Makefile.objs b/hw/Makefile.objs
index d770926ba9..c13b6ee0dd 100644
--- a/hw/Makefile.objs
+++ b/hw/Makefile.objs
@@ -8,6 +8,7 @@  devices-dirs-$(CONFIG_SOFTMMU) += char/
 devices-dirs-$(CONFIG_SOFTMMU) += cpu/
 devices-dirs-$(CONFIG_SOFTMMU) += display/
 devices-dirs-$(CONFIG_SOFTMMU) += dma/
+devices-dirs-$(CONFIG_SOFTMMU) += firmware/
 devices-dirs-$(CONFIG_SOFTMMU) += gpio/
 devices-dirs-$(CONFIG_HYPERV) += hyperv/
 devices-dirs-$(CONFIG_I2C) += i2c/
diff --git a/hw/firmware/Makefile.objs b/hw/firmware/Makefile.objs
new file mode 100644
index 0000000000..ea1f6d44df
--- /dev/null
+++ b/hw/firmware/Makefile.objs
@@ -0,0 +1 @@ 
+common-obj-y += uefi_edk2_crypto_policies.o
diff --git a/hw/firmware/uefi_edk2_crypto_policies.c b/hw/firmware/uefi_edk2_crypto_policies.c
new file mode 100644
index 0000000000..a0164272ea
--- /dev/null
+++ b/hw/firmware/uefi_edk2_crypto_policies.c
@@ -0,0 +1,209 @@ 
+/*
+ * UEFI EDK2 Support
+ *
+ * Copyright (c) 2019 Red Hat Inc.
+ *
+ * Author:
+ *  Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#include "qemu/osdep.h"
+#include "qapi/error.h"
+#include "qom/object_interfaces.h"
+#include "hw/firmware/uefi_edk2.h"
+
+
+#define TYPE_EDK2_CRYPTO "edk2_crypto"
+
+#define EDK2_CRYPTO_CLASS(klass) \
+     OBJECT_CLASS_CHECK(Edk2CryptoClass, (klass), \
+                        TYPE_EDK2_CRYPTO)
+#define EDK2_CRYPTO_GET_CLASS(obj) \
+     OBJECT_GET_CLASS(Edk2CryptoClass, (obj), \
+                      TYPE_EDK2_CRYPTO)
+#define EDK2_CRYPTO(obj) \
+     OBJECT_CHECK(Edk2Crypto, (obj), \
+                  TYPE_EDK2_CRYPTO)
+
+typedef struct FWCfgHostContent {
+    /*
+     * Path to the acceptable ciphersuites and the preferred order from
+     * the host-side crypto policy.
+     */
+    char *filename;
+    /*
+     * Add a new NAMED fw_cfg item as a raw "blob" of the given size. The data
+     * referenced by the starting pointer is only linked, NOT copied, into the
+     * data structure of the fw_cfg device.
+     */
+    char *contents;
+
+    size_t contents_length;
+} FWCfgHostContent;
+
+typedef struct Edk2Crypto {
+    Object parent_obj;
+
+    /*
+     * Path to the acceptable ciphersuites and the preferred order from
+     * the host-side crypto policy.
+     */
+    FWCfgHostContent ciphers;
+    /* Path to the trusted CA certificates configured on the host side. */
+    FWCfgHostContent cacerts;
+} Edk2Crypto;
+
+typedef struct Edk2CryptoClass {
+    ObjectClass parent_class;
+} Edk2CryptoClass;
+
+static Edk2Crypto *edk2_crypto_by_policy_id(const char *policy_id, Error **errp)
+{
+    Object *obj;
+
+    obj = object_resolve_path_component(object_get_objects_root(), policy_id);
+    if (!obj) {
+        error_setg(errp, "Cannot find EDK2 crypto policy ID %s", policy_id);
+        return NULL;
+    }
+
+    if (!object_dynamic_cast(obj, TYPE_EDK2_CRYPTO)) {
+        error_setg(errp, "Object '%s' is not a EDK2 crypto subclass",
+                         policy_id);
+        return NULL;
+    }
+
+    return EDK2_CRYPTO(obj);
+}
+
+static void edk2_crypto_prop_set_ciphers(Object *obj, const char *value,
+                                         Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->ciphers.filename);
+    s->ciphers.filename = g_strdup(value);
+}
+
+static char *edk2_crypto_prop_get_ciphers(Object *obj,
+                                          Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    return g_strdup(s->ciphers.filename);
+}
+
+static void edk2_crypto_prop_set_cacerts(Object *obj, const char *value,
+                                         Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->cacerts.filename);
+    s->cacerts.filename = g_strdup(value);
+}
+
+static char *edk2_crypto_prop_get_cacerts(Object *obj,
+                                          Error **errp G_GNUC_UNUSED)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    return g_strdup(s->cacerts.filename);
+}
+
+static void edk2_crypto_complete(UserCreatable *uc, Error **errp)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(uc);
+    Error *local_err = NULL;
+    GError *gerr = NULL;
+
+    if (s->ciphers.filename) {
+        if (!g_file_get_contents(s->ciphers.filename, &s->ciphers.contents,
+                                 &s->ciphers.contents_length, &gerr)) {
+            goto report_error;
+        }
+    }
+    if (s->cacerts.filename) {
+        if (!g_file_get_contents(s->cacerts.filename, &s->cacerts.contents,
+                                 &s->cacerts.contents_length, &gerr)) {
+            goto report_error;
+        }
+    }
+    return;
+
+ report_error:
+    error_setg(&local_err, "%s", gerr->message);
+    g_error_free(gerr);
+    error_propagate_prepend(errp, local_err, "EDK2 crypto policy: ");
+}
+
+static void edk2_crypto_finalize(Object *obj)
+{
+    Edk2Crypto *s = EDK2_CRYPTO(obj);
+
+    g_free(s->ciphers.filename);
+    g_free(s->ciphers.contents);
+    g_free(s->cacerts.filename);
+    g_free(s->cacerts.contents);
+}
+
+static void edk2_crypto_class_init(ObjectClass *oc, void *data)
+{
+    UserCreatableClass *ucc = USER_CREATABLE_CLASS(oc);
+
+    ucc->complete = edk2_crypto_complete;
+
+    object_class_property_add_str(oc, "ciphers",
+                                  edk2_crypto_prop_get_ciphers,
+                                  edk2_crypto_prop_set_ciphers,
+                                  NULL);
+    object_class_property_add_str(oc, "cacerts",
+                                  edk2_crypto_prop_get_cacerts,
+                                  edk2_crypto_prop_set_cacerts,
+                                  NULL);
+}
+
+static const TypeInfo edk2_crypto_info = {
+    .parent = TYPE_OBJECT,
+    .name = TYPE_EDK2_CRYPTO,
+    .instance_size = sizeof(Edk2Crypto),
+    .instance_finalize = edk2_crypto_finalize,
+    .class_size = sizeof(Edk2CryptoClass),
+    .class_init = edk2_crypto_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { TYPE_USER_CREATABLE },
+        { }
+    }
+};
+
+static void edk2_crypto_register_types(void)
+{
+    type_register_static(&edk2_crypto_info);
+}
+
+type_init(edk2_crypto_register_types);
+
+static void edk2_add_host_crypto_policy_https(FWCfgState *fw_cfg)
+{
+    Edk2Crypto *s;
+
+    s = edk2_crypto_by_policy_id("https", NULL);
+    if (!s) {
+        return;
+    }
+    if (s->ciphers.contents_length) {
+        fw_cfg_add_file(fw_cfg, "etc/edk2/https/ciphers",
+                        s->ciphers.contents, s->ciphers.contents_length);
+    }
+    if (s->cacerts.contents_length) {
+        fw_cfg_add_file(fw_cfg, "etc/edk2/https/cacerts",
+                        s->cacerts.contents, s->cacerts.contents_length);
+    }
+}
+
+void edk2_add_host_crypto_policy(FWCfgState *fw_cfg)
+{
+    edk2_add_host_crypto_policy_https(fw_cfg);
+}
diff --git a/include/hw/firmware/uefi_edk2.h b/include/hw/firmware/uefi_edk2.h
new file mode 100644
index 0000000000..f8f81c5cb2
--- /dev/null
+++ b/include/hw/firmware/uefi_edk2.h
@@ -0,0 +1,30 @@ 
+/*
+ * UEFI EDK2 Support
+ *
+ * Copyright (c) 2019 Red Hat Inc.
+ *
+ * Author:
+ *  Philippe Mathieu-Daudé <philmd@redhat.com>
+ *
+ * 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:
+ * @fw_cfg: fw_cfg device being modified
+ *
+ * Add a new named file containing the host crypto policy.
+ *
+ * This method is called by the machine_done() Notifier of
+ * some implementations of MachineState, currently the X86
+ * PCMachineState and the ARM VirtMachineState.
+ */
+void edk2_add_host_crypto_policy(FWCfgState *fw_cfg);
+
+#endif /* HW_FIRMWARE_UEFI_EDK2_H */