Re: [PATCH 09/15] common: tlv: Add TLV-Signature support

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

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 |