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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.05.2025 11:38, Ross Lagerwall wrote:
> @@ -79,6 +80,9 @@ static DEFINE_PER_CPU(struct tasklet, livepatch_tasklet);
>  static struct rsa_public_key builtin_payload_key;
>  #endif
>  
> +static int check_signature(const struct livepatch_elf *elf, void *raw,
> +                           size_t size);

I think it would be nice if this forward decl was avoided. Which looks to
be feasible if you moved the definition further up.

> @@ -1202,6 +1208,109 @@ static int load_payload_data(struct payload *payload, 
> void *raw, size_t len)
>      return rc;
>  }
>  
> +#ifdef CONFIG_PAYLOAD_VERIFY
> +#define MAX_SIG_NOTE_SIZE 1024
> +
> +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;
> +}
> +
> +static int check_signature(const struct livepatch_elf *elf, void *raw,
> +                           size_t size)
> +{
> +    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");
> +            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
> +static int check_signature(const struct livepatch_elf *elf, void *raw,
> +                           size_t size)

As indicated before, I also think it would be nice if this redundant
function header was eliminated, but changing the #if / #else / #endif
placement.

Jan