On 2025-11-03 12:41, Sascha Hauer wrote:
On Mon, Nov 03, 2025 at 12:21:56PM +0100, Jonas Rebmann wrote:
Hi Sascha,
On 2025-11-03 11:02, Sascha Hauer wrote:
On Tue, Oct 28, 2025 at 07:03:15PM +0100, Jonas Rebmann wrote:
Implement TLV signature using the existing placeholders for it. Use the
existing cryptographic primitives and public key handling used for
fitimage verification.
Signature is verified and then must be valid iff CONFIG_TLV_SIGNATURE is
enabled and a keyring is selected for the decoder. SHA256 hashing is
hardcoded for now.
As 16 bit are well sufficient to store the length of the signature
section in bytes, reduce it to its least significant 16 bit and reserve
the remaining 16 bit for future use.
As sig_len where the only reserved bits left, and where zero-reserved,
this leaves more wiggle room to still expand the format in the future.
Signed-off-by: Jonas Rebmann <[email protected]>
---
common/Kconfig | 5 +++
common/tlv/parser.c | 87
++++++++++++++++++++++++++++++++++++++++++++++++++++
include/tlv/format.h | 22 ++++++++++---
3 files changed, 109 insertions(+), 5 deletions(-)
diff --git a/common/Kconfig b/common/Kconfig
index d923d4c4b6..663465443d 100644
--- a/common/Kconfig
+++ b/common/Kconfig
@@ -1122,6 +1122,11 @@ 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
+ 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..cbf45413dd 100644
--- a/common/tlv/parser.c
+++ b/common/tlv/parser.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#define pr_fmt(fmt) "barebox-tlv: " fmt
+#include "tlv/format.h"
#include <common.h>
#include <tlv/tlv.h>
@@ -9,6 +10,80 @@
#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;
+ void *hash;
+
+ digest = digest_alloc_by_algo(algo);
+ if (!digest)
+ return -ENOMEM;
+
+ digest_init(digest);
+ if (IS_ERR(digest)) {
What you meant to do here is
ret = digest_init(digest);
if (ret) {
...
}
I used IS_ERR() here simply because that's how I saw it in
common/image-fit.c. Will that need to be changed too then?
I don't see that the return value of digest_init() is checked in
common/image-fit.c.
Yes, we should consistently check the return value of digest_init().
Anyway, my point was that your IS_ERR(digest) is bogus.
Oh indeed, common/image-fit.c uses IS_ERR(digest) for fit_alloc_digest()
but digest_init() is unchecked. Will apply your suggestion for v3.
+ for_each_public_key_keyring(key, id, 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: %pe!\n", spki_tlv, ERR_PTR(ret));
Not sure what this warning is about. Either it can happen that there are
two keys with the same hash in which case there's nothing to warn about,
or it can't happen and you would return an error here.
When this happens, there is either a 32-bit hash collision, or
something's broken, such as what we had last week, where ECDSA keys
where incorrectly initialized, leading to signature verification to
always fail even with a valid key.
We can't error out here already because when we designed the TLV
signature verification feature we specified that in case of spki hash
collision, all matching keys must be tried.
Ok, considering that a hash collision can happen we shouldn't warn about
it.
The odds of this happening are 1 in 2^32. It is much more likely that
the underlying issue lays elsewhere. If no spki hash match occurs,
there's a warning too:
if (!count_spki_matches) {
pr_warn("TLV spki %08x matched no key!\n", spki_tlv);
return -ENOKEY;
}
The difference between: 'There was no matching key' and 'We had a key
but failed to verify for dubious reasons' should clearly be reflected
in the logs.
Regards,
Jonas
--
Pengutronix e.K. | Jonas Rebmann |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
