Re: [PATCH v2 10/17] common: tlv: Add TLV-Signature support

[Jonas Rebmann <jre@pengutronix.de>]

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 <jre@pengutronix.de>
>>>> ---
>>>>    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    |