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

[Jan Beulich <jbeulich@xxxxxxxx>]

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,
> +                                     &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);

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