On 10/14/25 1:03 PM, Jonas Rebmann wrote:
> diff --git a/common/Kconfig b/common/Kconfig
> index d923d4c4b6..b7ac7094a6 100644
> --- a/common/Kconfig
> +++ b/common/Kconfig
> @@ -1122,6 +1122,10 @@ config TLV
> barebox TLV is a scheme for storing factory data on non-volatile
> storage. Unlike state, it's meant to be read-only.
>
> +config TLV_SIGNATURE
> + bool "barebox TLV signature support"
depends on TLV or open an if TLV to span this as well as TLV_DRV and
TLV_BAREBOX.
> + select CRYPTO_BUILTIN_KEYS
> +
> config TLV_DRV
> bool "barebox TLV generic driver"
> depends on TLV
> diff --git a/common/tlv/parser.c b/common/tlv/parser.c
> index f74ada99d7..0a23beba4e 100644
> --- a/common/tlv/parser.c
> +++ b/common/tlv/parser.c
> @@ -1,5 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
>
> +#include "tlv/format.h"
pr_fmt definition must be before any headers to ensure that it's seen
when pr_info is defined, even with future changes to headers.
> #define pr_fmt(fmt) "barebox-tlv: " fmt
>
> #include <common.h>
> @@ -9,6 +10,81 @@
> #include <linux/stat.h>
> #include <crc.h>
> #include <net.h>
> +#include <crypto/public_key.h>
> +
> +static int tlv_verify_try_key(const struct public_key *key, const uint8_t
> *sig,
> + const uint32_t sig_len, const void *data,
> + unsigned long data_len)
> +{
> + enum hash_algo algo = HASH_ALGO_SHA256;
> + int ret;
> + struct digest *digest = digest_alloc_by_algo(algo);
Make it either the last definition or move the assignment before the if.
> + void *hash;
> +
> + if (!digest)
> + return -ENOMEM;
> +
> + digest_init(digest);
> + if (IS_ERR(digest)) {
> + digest_free(digest);
> + return -EINVAL;
> + }
> + digest_update(digest, data, data_len);
> + hash = xzalloc(digest_length(digest));
> + digest_final(digest, hash);
> +
> + ret = public_key_verify(key, sig, sig_len, hash, algo);
> +
> + digest_free(digest);
> + free(hash);
> +
> + if (ret)
> + return -ENOKEY;
We have no strerror text for ENOKEY, but I believe it's a bit
out-of-place here anyway. -ENOKEY could be an error code from
tlv_verify, but when trying a specific key, -ENOKEY appears to be
inaccurate.
> + return 0;
> +}
> +
> +static int tlv_verify(struct tlv_header *header, const char *keyring)
> +{
> + const struct public_key *key;
> + size_t payload_sz = tlv_spki_hash_offset(header);
> + void *spki_tlv_ptr = (void *)header + payload_sz;
Can this be made const void *.
> + u32 spki_tlv = get_unaligned_le32(spki_tlv_ptr);
> + const int SPKI_LEN = 4;
Drop the const without a pointer (this isn't C++).
> + u32 sig_len = get_unaligned_be16(&header->length_sig);
> + int ret;
> + int count_spki_matches = 0;
> +
> + if (!IS_ENABLED(CONFIG_TLV_SIGNATURE)) {
> + pr_err("TLV signature selected in decoder but not enabled!\n");
Remove TLV prefix here and below, there's already a barebox-tlv: prefix.
> + return -ENOSYS;
> + } else if (sig_len == 0) {
> + pr_err("TLV signature selected in decoder but an unsigned TLV
> matched by magic %08x!\n", be32_to_cpu(header->magic));
> + return -EPROTO;
> + }
> + /* signature length field must always be zeroed during signage and
> verification */
s/signage/signing/
> + header->length_sig = 0;
> +
> + for_each_public_key_keyring(key, keyring) {
> + u32 spki_key = get_unaligned_le32(key->hash);
> +
> + if (spki_key == spki_tlv) {
> + count_spki_matches++;
> + ret = tlv_verify_try_key(key, spki_tlv_ptr + SPKI_LEN,
> sig_len - SPKI_LEN, header, payload_sz);
> + if (!ret)
> + return 0;
> + pr_warn("TLV spki %08x matched available key but
> signature verification failed: %s!\n", spki_tlv, strerror(-ret));
"%s", strerror(-ret) -> "%pE", PTR_ERR(ret)
> + }
> + }
> +
> + /* reset signature length field after verification to avoid later
> confusion */
> + put_unaligned_be16(sig_len, &header->length_sig);
> +
> + if (!count_spki_matches) {
> + pr_warn("TLV spki %08x matched no key!\n", spki_tlv);
> + return -ENOKEY;
> + }
> + return -EINVAL;
> +}
>
> int tlv_parse(struct tlv_device *tlvdev,
> const struct tlv_decoder *decoder)
> @@ -17,6 +93,7 @@ int tlv_parse(struct tlv_device *tlvdev,
> struct tlv_mapping *map = NULL;
> struct tlv_header *header = tlv_device_header(tlvdev);
> u32 magic;
> + u16 reserved;
> size_t size;
> int ret = 0;
> u32 crc = ~0;
> @@ -24,6 +101,7 @@ int tlv_parse(struct tlv_device *tlvdev,
> magic = be32_to_cpu(header->magic);
>
> size = tlv_total_len(header);
> + reserved = get_unaligned_be16(&header->reserved);
>
> if (size == SIZE_MAX) {
> pr_warn("Invalid TLV header, overflows\n");
> @@ -36,6 +114,12 @@ int tlv_parse(struct tlv_device *tlvdev,
> return -EILSEQ;
> }
>
> + if (decoder->signature_keyring) {
> + ret = tlv_verify(header, decoder->signature_keyring);
> + if (ret)
> + return ret;
> + }
> +
> for_each_tlv(header, tlv) {
> struct tlv_mapping **mappings;
> u16 tag = TLV_TAG(tlv);
> diff --git a/include/tlv/format.h b/include/tlv/format.h
> index c4521c3131..cbe0a132b1 100644
> --- a/include/tlv/format.h
> +++ b/include/tlv/format.h
> @@ -47,11 +47,12 @@ struct tlv {
> struct tlv_header {
> __be32 magic;
> __be32 length_tlv; /* in bytes */
> - __be32 length_sig; /* in bytes */
> + __be16 reserved;
> + __be16 length_sig; /* in bytes */
> struct tlv tlvs[];
> - /* __be32 crc; */
> /* u8 sig[]; */
> -};
> + /* __be32 crc; */
> +} __packed;
> static_assert(sizeof(struct tlv_header) == 3 * 4);
>
> #define for_each_tlv(tlv_head, tlv) \
> @@ -62,8 +63,19 @@ static inline size_t tlv_total_len(const struct tlv_header
> *header)
> size_t ret;
>
> ret = size_add(sizeof(struct tlv_header),
> get_unaligned_be32(&header->length_tlv));
> - ret = size_add(ret, sizeof(__be32)); /* CRC appended after TLVs */
> - ret = size_add(ret, get_unaligned_be32(&header->length_sig)); /*
> optional signature appended after CRC */
> + ret = size_add(ret, get_unaligned_be16(&header->length_sig)); /*
> optional signature appended after TLVs */
> + ret = size_add(ret, sizeof(__be32)); /* CRC at end of file */
> +
> + return ret; /* SIZE_MAX on overflow */
> +}
> +
> +/*
> + * Retrieve length of header+TLVs (offset of spki hash part of signature if
> available)
> + */
> +
> +static inline size_t tlv_spki_hash_offset(const struct tlv_header *header)
> +{
> + size_t ret = size_add(sizeof(struct tlv_header),
> get_unaligned_be32(&header->length_tlv));
>
> return ret; /* SIZE_MAX on overflow */
Shouldn't you then check for SIZE_MAX at callsites?
Cheers,
Ahmad
> }
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
