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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Fri, Jun 20, 2025 at 11:31 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Mon, Jun 02, 2025 at 02:36:37PM +0100, Ross Lagerwall wrote:
> > From: Jennifer Herbert <jennifer.herbert@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Signed-off-by: Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>
> > ---
> >
> > 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.

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