On Mon, Jun 02, 2025 at 02:36:37PM +0100, Ross Lagerwall wrote:
> From: Jennifer Herbert <jennifer.herb...@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.herb...@cloud.com>
> Signed-off-by: Ross Lagerwall <ross.lagerw...@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,
> + ¬e);
> + 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, ¬e);
> + 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.
