Re: [PATCH 3/4] livepatch: Embed public key in Xen

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 16:32, Ross Lagerwall wrote:
> From: Kevin Lampis <klampis@xxxxxxxxx>
> 
> Make it possible to embed a public key in Xen to be used when verifying
> live patch payloads. Inclusion of the public key is optional.
> 
> To avoid needing to include a DER / X.509 parser in the hypervisor, the
> public key is unpacked at build time and included in a form that is
> convenient for the hypervisor to consume. This is different approach
> from that used by Linux which embeds the entire X.509 certificate and
> builds in a parser for it.
> 
> A suitable key can be created using openssl:
> 
> openssl req -x509 -newkey rsa:2048 -keyout priv.pem -out pub.pem \
>     -sha256 -days 3650 -nodes \
>     -subj 
> "/C=XX/ST=StateName/L=CityName/O=CompanyName/OU=CompanySectionName/CN=CommonNameOrHostname"
> openssl x509 -inform PEM -in pub.pem -outform PEM -pubkey -nocert -out 
> crypto/signing_key.pem

According to this the .pem file isn't really a source one; how does ...

> --- a/xen/common/Kconfig
> +++ b/xen/common/Kconfig
> @@ -481,6 +481,24 @@ config LIVEPATCH
>  
>         If unsure, say Y.
>  
> +config PAYLOAD_SIGNING
> +     bool "Verify signed LivePatch payloads"
> +     depends on LIVEPATCH
> +     select CRYPTO
> +     help
> +       Verify signed LivePatch payloads using an RSA public key built
> +       into the Xen hypervisor. Selecting this option requires a
> +       public key in PEM format to be available for embedding during
> +       the build.
> +
> +config PAYLOAD_SIG_KEY
> +     string "File name of payload signing public key"
> +     default "signing_key.pem"

... this work in an out-of-tree build?

> +     depends on PAYLOAD_SIGNING

As to the name of this: According to its description, it's signature
verification, not signing. I think this wants reflecting in the name.

> --- a/xen/common/Makefile
> +++ b/xen/common/Makefile
> @@ -28,7 +28,7 @@ obj-$(CONFIG_LIVEPATCH) += livepatch.o livepatch_elf.o
>  obj-$(CONFIG_LLC_COLORING) += llc-coloring.o
>  obj-$(CONFIG_VM_EVENT) += mem_access.o
>  obj-y += memory.o
> -obj-y += mpi.o
> +obj-$(CONFIG_PAYLOAD_SIGNING) += mpi.o

Looks like the Kconfig symbol then wants introducing in the earlier
patch, or as a prereq thereto.

> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -11,6 +11,8 @@
>  #include <xen/lib.h>
>  #include <xen/list.h>
>  #include <xen/mm.h>
> +#include <xen/mpi.h>
> +#include <xen/rsa.h>
>  #include <xen/sched.h>
>  #include <xen/smp.h>
>  #include <xen/softirq.h>
> @@ -73,6 +75,12 @@ static struct livepatch_work livepatch_work;
>  static DEFINE_PER_CPU(bool, work_to_do);
>  static DEFINE_PER_CPU(struct tasklet, livepatch_tasklet);
>  
> +#ifdef CONFIG_PAYLOAD_SIGNING
> +/* The public key contained with Xen used to verify payload signatures. */
> +extern const uint8_t xen_livepatch_key_data[];

Iirc Misra demands that declarations appear in headers.

> +static struct rsa_public_key builtin_payload_key;
> +#endif
> +
>  static int get_name(const struct xen_livepatch_name *name, char *n)
>  {
>      if ( !name->size || name->size > XEN_LIVEPATCH_NAME_SIZE )
> @@ -2287,6 +2295,34 @@ static void cf_check livepatch_printall(unsigned char 
> key)
>      spin_unlock(&payload_lock);
>  }
>  
> +#ifdef CONFIG_PAYLOAD_SIGNING

Nit: The #ifdef would better appear inside the function body, to
reduce redundancy.

> +static int __init load_builtin_payload_key(void)
> +{
> +    const uint8_t *ptr;
> +    uint32_t len;
> +
> +    rsa_public_key_init(&builtin_payload_key);
> +
> +    ptr = xen_livepatch_key_data;

This being the sole place where the array is used, ...

> @@ -2305,6 +2341,11 @@ static struct notifier_block cpu_nfb = {
>  static int __init cf_check livepatch_init(void)
>  {
>      unsigned int cpu;
> +    int err;
> +
> +    err = load_builtin_payload_key();
> +    if (err)

(Nit: style)

> --- a/xen/crypto/Makefile
> +++ b/xen/crypto/Makefile
> @@ -1,3 +1,15 @@
>  obj-y += rijndael.o
> -obj-y += rsa.o
> +obj-$(CONFIG_PAYLOAD_SIGNING) += rsa.o
>  obj-y += vmac.o
> +
> +obj-$(CONFIG_PAYLOAD_SIGNING) += builtin_payload_key.o
> +
> +ifeq ($(CONFIG_PAYLOAD_SIGNING),y)
> +key_path := $(srctree)/crypto/$(patsubst "%",%,$(CONFIG_PAYLOAD_SIG_KEY))
> +$(obj)/builtin_payload_key.bin: $(key_path) $(srctree)/tools/extract-key.py
> +     $(srctree)/tools/extract-key.py < $< > $@.new
> +     $(call move-if-changed,$@.new,$@)
> +
> +$(obj)/builtin_payload_key.S: $(srctree)/tools/binfile 
> $(obj)/builtin_payload_key.bin FORCE
> +     $(call if_changed,binfile,$(obj)/builtin_payload_key.bin 
> xen_livepatch_key_data)

... arrangements want making for the array to live ideally in .init.rodata,
but at least somewhere in .init.*. E.g. by passing -i to tools/binfile.

Jan