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

[Sascha Hauer <s.hauer@pengutronix.de>]

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.

> > > +	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.

> > > +	if (decoder->signature_keyring) {
> > > +		ret = tlv_verify(header, decoder->signature_keyring);
> > > +		if (ret)
> > > +			return ret;
> > 
> > Does this mean I can bypass the verification just by putting some
> > unsigned TLV data with TLV_MAGIC_BAREBOX_V1 into the TLV partition?
> 
> If barebox_tlv_v1 or another unsigned decoder is enabled, its compatible
> needs to be referenced in a devicetree node in order for unsigned TLVs
> to be parseable (except when using the "TLV" command which does create a
> new devicetree node named tlv<num>).
> 
> Even if both signed and unsigned decoder are both enabled and referenced
> in the devicetree (and I can see applications where this may be
> desirable), feeding the device with unsigned TLV data never lets you
> modify the devicetree node that has a compatible string of a signed TLV
> decoder.

I see. I hoped I missed something :)

Sascha

-- 
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 |