[PATCH v2 4/5] livepatch: Load built-in key during boot

Ross Lagerwall posted 5 patches 7 months ago
There is a newer version of this series
[PATCH v2 4/5] livepatch: Load built-in key during boot
Posted by Ross Lagerwall 7 months ago
Parse the raw data of the embedded RSA key into a form that can be later
used for verifying live patch signatures.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---

In v2:

* Split out from "livepatch: Embed public key in Xen"

 xen/common/livepatch.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c
index be9b7e367553..bc158971b4bf 100644
--- 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,10 @@ 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_VERIFY
+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 +2293,31 @@ static void cf_check livepatch_printall(unsigned char key)
     spin_unlock(&payload_lock);
 }
 
+static int __init load_builtin_payload_key(void)
+{
+#ifdef CONFIG_PAYLOAD_VERIFY
+    const uint8_t *ptr;
+    uint32_t len;
+
+    rsa_public_key_init(&builtin_payload_key);
+
+    ptr = xen_livepatch_key_data;
+
+    memcpy(&len, ptr, sizeof(len));
+    ptr += sizeof(len);
+    builtin_payload_key.n = mpi_read_raw_data(ptr, len);
+    ptr += len;
+
+    memcpy(&len, ptr, sizeof(len));
+    ptr += sizeof(len);
+    builtin_payload_key.e = mpi_read_raw_data(ptr, len);
+
+    return rsa_public_key_prepare(&builtin_payload_key);
+#else
+    return 0;
+#endif
+}
+
 static int cf_check cpu_callback(
     struct notifier_block *nfb, unsigned long action, void *hcpu)
 {
@@ -2305,6 +2336,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)
+        return err;
 
     for_each_online_cpu ( cpu )
     {
-- 
2.49.0
Re: [PATCH v2 4/5] livepatch: Load built-in key during boot
Posted by Jan Beulich 7 months ago
On 15.05.2025 11:38, Ross Lagerwall wrote:
> @@ -73,6 +75,10 @@ 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_VERIFY
> +static struct rsa_public_key builtin_payload_key;

__read_mostly or even __ro_after_init?

> @@ -2287,6 +2293,31 @@ static void cf_check livepatch_printall(unsigned char key)
>      spin_unlock(&payload_lock);
>  }
>  
> +static int __init load_builtin_payload_key(void)
> +{
> +#ifdef CONFIG_PAYLOAD_VERIFY
> +    const uint8_t *ptr;
> +    uint32_t len;
> +
> +    rsa_public_key_init(&builtin_payload_key);
> +
> +    ptr = xen_livepatch_key_data;
> +
> +    memcpy(&len, ptr, sizeof(len));

Doesn't this (and the similar one further down) need to be an endian-
ness conversion? In fact, seeing how the data is being built, it's
not really clear to me what endian-ness the field would be written
in, when build host and target host endianness differ.

> +    ptr += sizeof(len);
> +    builtin_payload_key.n = mpi_read_raw_data(ptr, len);

Whether there are endianness concerns here I also don't know.

> @@ -2305,6 +2336,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): Missing blanks.

Jan
Re: [PATCH v2 4/5] livepatch: Load built-in key during boot
Posted by Ross Lagerwall 6 months, 2 weeks ago
On Sun, May 18, 2025 at 1:17 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.05.2025 11:38, Ross Lagerwall wrote:
> > @@ -73,6 +75,10 @@ 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_VERIFY
> > +static struct rsa_public_key builtin_payload_key;
>
> __read_mostly or even __ro_after_init?
>
> > @@ -2287,6 +2293,31 @@ static void cf_check livepatch_printall(unsigned char key)
> >      spin_unlock(&payload_lock);
> >  }
> >
> > +static int __init load_builtin_payload_key(void)
> > +{
> > +#ifdef CONFIG_PAYLOAD_VERIFY
> > +    const uint8_t *ptr;
> > +    uint32_t len;
> > +
> > +    rsa_public_key_init(&builtin_payload_key);
> > +
> > +    ptr = xen_livepatch_key_data;
> > +
> > +    memcpy(&len, ptr, sizeof(len));
>
> Doesn't this (and the similar one further down) need to be an endian-
> ness conversion? In fact, seeing how the data is being built, it's
> not really clear to me what endian-ness the field would be written
> in, when build host and target host endianness differ.

Good point. In v3, I've made this and the build-time tool use little
endian which in most cases should be a no-op.

>
> > +    ptr += sizeof(len);
> > +    builtin_payload_key.n = mpi_read_raw_data(ptr, len);
>
> Whether there are endianness concerns here I also don't know.

As a byte sequence, this always uses big endian (much like OpenSSL).
I've added a comment in v3 to clarify this.

Thanks,
Ross