Re: [PATCH v3 5/5] livepatch: Verify livepatch signatures

["Roger Pau Monné" <roger.pau@citrix.com>]

On Mon, Jun 02, 2025 at 02:36:37PM +0100, Ross Lagerwall wrote:
> From: Jennifer Herbert <jennifer.herbert@cloud.com>
> 
> Verify livepatch signatures against the embedded public key in Xen.
> Failing to verify does not prevent the livepatch from being loaded.
> In future, this will be changed for certain cases (e.g. when Secure Boot
> is enabled).
> 
> Signed-off-by: Jennifer Herbert <jennifer.herbert@cloud.com>
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
> 
> In v3:
> 
> * Minor style fixes
> 
>  xen/common/livepatch.c          | 103 ++++++++++++++++++++++++++++++++
>  xen/common/livepatch_elf.c      |  55 +++++++++++++++++
>  xen/include/xen/livepatch.h     |  10 ++++
>  xen/include/xen/livepatch_elf.h |  18 ++++++
>  4 files changed, 186 insertions(+)
> 
> diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
> index 92d1d342d872..56a3d125483f 100644
> --- a/xen/common/livepatch.c
> +++ b/xen/common/livepatch.c
> @@ -14,6 +14,7 @@
>  #include <xen/mpi.h>
>  #include <xen/rsa.h>
>  #include <xen/sched.h>
> +#include <xen/sha2.h>
>  #include <xen/smp.h>
>  #include <xen/softirq.h>
>  #include <xen/spinlock.h>
> @@ -525,6 +526,106 @@ static int check_xen_buildid(const struct livepatch_elf *elf)
>      return 0;
>  }
>  
> +#ifdef CONFIG_PAYLOAD_VERIFY
> +static int check_rsa_sha256_signature(void *data, size_t datalen,
> +                                      void *sig, uint32_t siglen)

I think both data and sig could be const here?

> +{
> +    struct sha2_256_state hash;
> +    MPI s;
> +    int rc;
> +
> +    s = mpi_read_raw_data(sig, siglen);
> +    if ( !s )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "Failed to mpi_read_raw_data\n");
> +        return -ENOMEM;
> +    }
> +
> +    sha2_256_init(&hash);
> +    sha2_256_update(&hash, data, datalen);
> +
> +    rc = rsa_sha256_verify(&builtin_payload_key, &hash, s);
> +    if ( rc )
> +        printk(XENLOG_ERR LIVEPATCH "rsa_sha256_verify failed: %d\n", rc);
> +
> +    mpi_free(s);
> +
> +    return rc;
> +}
> +#endif
> +
> +static int check_signature(const struct livepatch_elf *elf, void *raw,
> +                           size_t size)
> +{
> +#ifdef CONFIG_PAYLOAD_VERIFY
> +#define MAX_SIG_NOTE_SIZE 1024
> +    static const char notename[] = "Xen";
> +    void *sig;
> +    livepatch_elf_note note;
> +    int rc;
> +
> +    rc = livepatch_elf_note_by_names(elf, ELF_XEN_SIGNATURE, notename, -1,
> +                                     &note);
> +    if ( rc )
> +    {
> +        dprintk(XENLOG_DEBUG, LIVEPATCH "%s: Signature not present\n",
> +                elf->name);
> +        return rc;
> +    }
> +
> +    /* We expect only one signature, find a second is an error! */
> +    rc = livepatch_elf_next_note_by_name(notename, -1, &note);
> +    if ( rc != -ENOENT )
> +    {
> +        if ( rc )
> +        {
> +            printk(XENLOG_ERR LIVEPATCH
> +                   "Error while checking for notes! err = %d\n", rc);
> +            return rc;
> +        }
> +        else
> +        {
> +            printk(XENLOG_ERR LIVEPATCH
> +                   "Error, found second signature note! There can be only one!\n");

FWIW, it's a nit, but I think there are too many exclamation marks in
the error message above:

"Error, multiple signatures found\n"

Would be better.

I'm also wondering, would it make sense to allow multiple signatures
to be present?  I guess not because livepatches are built for specific
Xen binaries, and then we know exactly which key is embedded there.  A
livepatch would never apply to two different builds with different
keys.

> +            return -EINVAL;
> +        }
> +    }
> +
> +    if ( SIGNATURE_VERSION(note.type) != LIVEPATCH_SIGNATURE_VERSION ||
> +         SIGNATURE_ALGORITHM(note.type) != SIGNATURE_ALGORITHM_RSA ||
> +         SIGNATURE_HASH(note.type) != SIGNATURE_HASH_SHA256 )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH
> +               "Unsupported signature type: v:%u, a:%u, h:%u\n",
> +               SIGNATURE_VERSION(note.type), SIGNATURE_ALGORITHM(note.type),
> +               SIGNATURE_HASH(note.type));
> +        return -EINVAL;
> +    }
> +
> +    if ( note.size == 0 || note.size >= MAX_SIG_NOTE_SIZE )
> +    {
> +        printk(XENLOG_ERR LIVEPATCH "Invalid signature note size: %u\n",
> +               note.size);
> +        return -EINVAL;
> +    }
> +
> +    sig = xmalloc_bytes(note.size);
> +    if ( !sig )
> +        return -ENOMEM;
> +
> +    memcpy(sig, note.data, note.size);
> +
> +    /* Remove signature from data, as can't be verified with it. */
> +    memset((void *)note.data, 0, note.size);
> +    rc = check_rsa_sha256_signature(raw, size, sig, note.size);
> +
> +    xfree(sig);
> +    return rc;
> +#else
> +    return -EINVAL;

EOPNOTSUPP would be more accurate here.

> +#endif
> +}
> +
>  static int check_special_sections(const struct livepatch_elf *elf)
>  {
>      unsigned int i;
> @@ -1162,6 +1263,8 @@ static int load_payload_data(struct payload *payload, void *raw, size_t len)
>      if ( rc )
>         goto out;
>  
> +    check_signature(&elf, raw, len);

Wouldn't it make more sense to unconditionally fail here if
CONFIG_PAYLOAD_VERIFY is enabled and signature verification fails?

IOW: if you built an hypervisor with PAYLOAD_VERIFY enabled signature
verification needs to be enforced.

> +
>      rc = move_payload(payload, &elf);
>      if ( rc )
>          goto out;
> diff --git a/xen/common/livepatch_elf.c b/xen/common/livepatch_elf.c
> index 25ce1bd5a0ad..b27c4524611d 100644
> --- a/xen/common/livepatch_elf.c
> +++ b/xen/common/livepatch_elf.c
> @@ -23,6 +23,61 @@ livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
>      return NULL;
>  }
>  
> +int livepatch_elf_note_by_names(const struct livepatch_elf *elf,
> +                                const char *sec_name, const char *note_name,
> +                                const unsigned int type,
> +                                livepatch_elf_note *note)
> +{
> +     const struct livepatch_elf_sec *sec = livepatch_elf_sec_by_name(elf,
> +                                                                     sec_name);
> +     if ( !sec )
> +           return -ENOENT;
> +
> +     note->end = sec->addr + sec->sec->sh_size;
> +     note->next = sec->addr;
> +
> +     return livepatch_elf_next_note_by_name(note_name, type, note);
> +}
> +
> +int livepatch_elf_next_note_by_name(const char *note_name,
> +                                    const unsigned int type,
> +                                    livepatch_elf_note *note)
> +{
> +     const Elf_Note *pkd_note = note->next;
> +     size_t notenamelen = strlen(note_name) + 1;
> +     size_t note_hd_size;
> +     size_t note_size;
> +     size_t remaining;
> +
> +     while ( (void *)pkd_note < note->end )
> +     {
> +

Stray newline?

remaining and note_hd_size can be defined inside of the loop to reduce
the scope.

> +         remaining = note->end - (void *)pkd_note;
> +         if ( remaining < sizeof(livepatch_elf_note) )
> +             return -EINVAL;
> +
> +         note_hd_size = sizeof(Elf_Note) + ((pkd_note->namesz + 3) & ~0x3);
> +         note_size = note_hd_size + ((pkd_note->descsz + 3) & ~0x3);

Could be have a macro or similar for this ((x + 3) & ~0x3)
manipulation?

> +         if ( remaining < note_size )
> +             return -EINVAL;

ENOSPC might be less ambiguous than EINVAL for both the above?

> +
> +         if ( notenamelen == pkd_note->namesz &&
> +              !memcmp(note_name, (const void *) pkd_note + sizeof(Elf_Note),
> +                      notenamelen) &&
> +              (type == -1 || pkd_note->type == type) )
> +         {
> +             note->size = pkd_note->descsz;
> +             note->type = pkd_note->type;
> +             note->data = (void *)pkd_note + note_hd_size;
> +             note->next = (void *)pkd_note + note_size;
> +             return 0;
> +         }
> +         pkd_note = (void *)pkd_note + note_size;
> +     }

It's a nit, but I would write this as a for loop, as it's clearer then
the index advance:

for ( pkd_note = note->next; (void *)pkd_note < note->end;
      pkd_note = (void *)pkd_note + note_size )
{
...

> +
> +     return -ENOENT;
> +}
> +
>  static int elf_verify_strtab(const struct livepatch_elf_sec *sec)
>  {
>      const Elf_Shdr *s;
> diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h
> index 52f90cbed45b..12206ce3b2b8 100644
> --- a/xen/include/xen/livepatch.h
> +++ b/xen/include/xen/livepatch.h
> @@ -38,6 +38,7 @@ struct xen_sysctl_livepatch_op;
>  #define ELF_LIVEPATCH_DEPENDS     ".livepatch.depends"
>  #define ELF_LIVEPATCH_XEN_DEPENDS ".livepatch.xen_depends"
>  #define ELF_BUILD_ID_NOTE         ".note.gnu.build-id"
> +#define ELF_XEN_SIGNATURE         ".note.Xen.signature"
>  #define ELF_LIVEPATCH_LOAD_HOOKS      ".livepatch.hooks.load"
>  #define ELF_LIVEPATCH_UNLOAD_HOOKS    ".livepatch.hooks.unload"
>  #define ELF_LIVEPATCH_PREAPPLY_HOOK   ".livepatch.hooks.preapply"
> @@ -49,6 +50,15 @@ struct xen_sysctl_livepatch_op;
>  /* Arbitrary limit for payload size and .bss section size. */
>  #define LIVEPATCH_MAX_SIZE     MB(2)
>  
> +#define SIGNATURE_VERSION(type) ((type) & 0xffff)
> +#define LIVEPATCH_SIGNATURE_VERSION 1
> +
> +#define SIGNATURE_ALGORITHM(type) (((type) >> 16) & 0xff)
> +#define SIGNATURE_ALGORITHM_RSA 0
> +
> +#define SIGNATURE_HASH(type) (((type) >> 24) & 0xff)
> +#define SIGNATURE_HASH_SHA256 0

You could possibly use MASK_EXTR() for the macros above?

Thanks, Roger.