[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v3 5/5] livepatch: Verify livepatch signatures
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |