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)
+{
+ 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");
+ 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;
+#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);
+
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 )
+ {
+
+ 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);
+ if ( remaining < note_size )
+ return -EINVAL;
+
+ 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;
+ }
+
+ 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
+
struct livepatch_symbol {
const char *name;
unsigned long value;
diff --git a/xen/include/xen/livepatch_elf.h b/xen/include/xen/livepatch_elf.h
index 842111e14518..04611bac410e 100644
--- a/xen/include/xen/livepatch_elf.h
+++ b/xen/include/xen/livepatch_elf.h
@@ -39,6 +39,16 @@ struct livepatch_elf {
unsigned int symtab_idx;
};
+typedef struct {
+ uint32_t size; /* Note size */
+ uint32_t type; /* Note type */
+ const void *data; /* Pointer to note */
+
+ /* Private */
+ const Elf_Note *next;
+ const void *end;
+} livepatch_elf_note;
+
const struct livepatch_elf_sec *
livepatch_elf_sec_by_name(const struct livepatch_elf *elf,
const char *name);
@@ -48,6 +58,14 @@ void livepatch_elf_free(struct livepatch_elf *elf);
int livepatch_elf_resolve_symbols(struct livepatch_elf *elf);
int livepatch_elf_perform_relocs(struct livepatch_elf *elf);
+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);
+int livepatch_elf_next_note_by_name(const char *note_name,
+ const unsigned int type,
+ livepatch_elf_note *note);
+
static inline bool livepatch_elf_ignore_section(const Elf_Shdr *sec)
{
return !(sec->sh_flags & SHF_ALLOC);
--
2.49.0On 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,
> + ¬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.
On Fri, Jun 20, 2025 at 11:31 AM Roger Pau Monné <roger.pau@citrix.com> wrote:
>
> 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,
> > + ¬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.
I agree that while it could be possible, I don't really see a valid use
for it. This restriction could be relaxed in future if it is deemed
useful.
>
> > + 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.
As per the commit message, this doesn't actually enforce anything yet.
The intention is to tie signature enforcement to lockdown mode once
those patches have been merged.
Ross
On 02.06.2025 15:36, Ross Lagerwall wrote:
> @@ -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)
> +{
> + struct sha2_256_state hash;
> + MPI s;
> + int rc;
> +
> + s = mpi_read_raw_data(sig, siglen);
sig apparently being an input, can't the function parameter be pointer-to-const?
> + if ( !s )
> + {
> + printk(XENLOG_ERR LIVEPATCH "Failed to mpi_read_raw_data\n");
Both there and ...
> + 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);
... here: Instead of mentioning function names, perhaps better to say in normal
word what failed. E.g. here "RSA/SHA256 verification failed".
> + mpi_free(s);
> +
> + return rc;
> +}
> +#endif
> +
> +static int check_signature(const struct livepatch_elf *elf, void *raw,
"raw" is an input only, too, isn't it? As such it ought to be const void *.
> + 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);
I can see why finding another signature of the same type might be a problem,
but one of a different type should be fine (i.e. unambiguous), I suppose.
Irrespective I find it slightly odd that you pass in a pointer to the same
local variable. The function, after all, is free to alter that variable
ahead of encountering the next matching 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");
> + 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);
In principle new code is supposed to be using functions from the xvmalloc()
family. (That family deliberately does not include xvmalloc_bytes(). I think
you mean xmalloc_array() here anyway.)
> @@ -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);
You entirely ignore the return value? (I was first wondering why the stub variant
of the function would return -EINVAL rather than 0.)
> --- 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,
So the plural in the function name is because it's both a section and a note
name, whereas ...
> + 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;
(Nit: Too deep indentation.)
> + 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)
... here you only look for further instances in the previously found section.
What if there's a 2nd section of the given name?
livepatch_elf_sec_by_name() also doesn't look to be making sure the section is
of the correct type.
Why is it necessary o pass in the note name again? A typical "find next" would
find the same kind as the "find first" did. Whereas here by passing in a
different name one can achieve "interesting" results. (Getting this correct
may not be necessary, but a comment would then want putting ahead of the
function.)
> +{
> + 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 )
Misra objects to the casting away of type qualifiers (and I do, too). More such
further down (and at least one instance also further up).
> + {
> +
Nit: Stray blank line. Perhaps you really meant to move some of the variable
declarations here?
> + 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);
What use are the 0x prefixes here (and then only on 2 of the three instances)?
> + if ( remaining < note_size )
> + return -EINVAL;
> +
> + if ( notenamelen == pkd_note->namesz &&
> + !memcmp(note_name, (const void *) pkd_note + sizeof(Elf_Note),
Nit: Stray blank between cast and cast value.
Jan
© 2016 - 2025 Red Hat, Inc.