Re: [PATCH v2] tlv: Add tlv_bind_soc_uid mapping

[Jonas Rebmann <jre@pengutronix.de>]

Hi Sascha,

On 2025-11-18 09:40, Sascha Hauer wrote:
> On Mon, Nov 17, 2025 at 06:14:06PM +0100, Jonas Rebmann wrote:
>> Particularly when using secure boot with signed TLVs, it may be required
>> to issue and sign TLVs for specific units. As typically all units of a
>> board are compiled to validate TLVs against the same key, a "binding"
>> mechanism is needed if interchange of TLVs across those units must be
>> prevented. This mapping binds against the UID of the SoC, rendering a
>> signed TLV with such a field invalid for all but the one unit.
>>
>> When generating TLVs that use this mapping, the exact case-sensitive
>> string representation of the SoC UID must be taken into account.
> 
> Do we really want to have this case-sensitive? I am not sure we're not
> creating problems with this once somebody changes the case for
> compatibility with the kernel, it was accidently wrong etc.

To me the big question is: What is a SoC UID?

Is it an arbitrary string that happens to be, for many SoCs composed of
[0-9A-F] and efficiently represented in binary in the efuses? Then it
feels a bit surprising to me to compare this 'arbitrary vendor-provided
string' case-insensitively.

But if we consider this an arbitrary block of binary data, typically
looked at in hexadecimal then I suggest we use the raw "bytes"-format I
sent an RFC patch for on Nov 12, and compare to
barebox_get_soc_uid_bin(). I originally wrote that RFC patch for storing
SoC UIDs but had a conversation with Ahmad that led me to view the SoC
UID as an arbitrary string. However now that we have
barebox_get_soc_uid_bin(), I'm tempted to change my mind.

>> Add the special mapping tlv_bind_soc_uid that aborts TLV parsing if the
>> supplied string does not match the SoC UID number.
>>
>> Include this mapping in barebox_tlv_v1_mappings with tag 0x0024 to make
>> it available in testing and in other setups using the generic tlv
>> parsers.
>>
>> Set up tlv_register_default as a late initcall so that it's loaded after
>> the SoC UID was initialized.
>>
>> Signed-off-by: Jonas Rebmann <jre@pengutronix.de>
>> ---
>> Changes in v2:
>> - Switch to using barebox_get_soc_uid and rename and reword everything
>>    accordingly (serial number -> soc uid)
>> - Init tlv_register_default as late_initcall instead of device_initcall
>> - Link to v1: https://lore.barebox.org/barebox/20251112-tlv_bind_serial-v1-1-638cf222553a@pengutronix.de
>> ---
>>   common/tlv/barebox.c | 18 +++++++++++++++++-
>>   include/tlv/tlv.h    |  1 +
>>   2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/tlv/barebox.c b/common/tlv/barebox.c
>> index 24de3eeaaa..fdba9fa2a5 100644
>> --- a/common/tlv/barebox.c
>> +++ b/common/tlv/barebox.c
>> @@ -1,8 +1,12 @@
>>   // SPDX-License-Identifier: GPL-2.0-only
>>   
>> +#include "barebox-info.h"
>>   #include <common.h>
>>   #include <net.h>
>>   #include <tlv/tlv.h>
>> +#include <param.h>
>> +#include <string.h>
>> +
>>   
>>   int tlv_handle_serial(struct tlv_device *dev, struct tlv_mapping *map, u16 len, const u8 *val)
>>   {
>> @@ -16,6 +20,16 @@ int tlv_handle_serial(struct tlv_device *dev, struct tlv_mapping *map, u16 len,
>>   	return 0;
>>   }
>>   
>> +int tlv_bind_soc_uid(struct tlv_device *dev, struct tlv_mapping *map, u16 len, const u8 *val)
>> +{
>> +	char *tlv_serial = basprintf("%.*s", len, val);
> 
> tlv_serial is not freed.

I'm just doing the same here as all other handlers ("handle"s?) in
common/tlv/barebox.c do. The string representation of the TLV field is
consumed by __tlv_format eventually:

   param->value = buf; /* pass ownership */

So not freeing seems correct here.

>> +
>> +	if (streq_ptr(tlv_serial, barebox_get_soc_uid()))
>> +		return __tlv_format_str(dev, map, len, val) ? 0 : -ENOMEM;
> 
> Why not simply forward the return value __tlv_format_str() instead?
> (which is 0 or -ENOMEM anyway).

Here too I'm doing the same as the other handlers do. __tlv_format with
the underscores returns buf upon success, NULL on error and it seems
right that this is probably (yet not guaranteed to be) ENOMEM then.
Anyway all the handlers return ENOMEM if there's an error in
__tlv_format either because they call via the macro

   #define tlv_format(tlvdev, map, ...) ({ __tlv_format(tlvdev, map, basprintf(__VA_ARGS__)) ? 0 : -ENOMEM; })

Or, as tlv_format_str, check directly:

   int tlv_format_str(struct tlv_device *dev, struct tlv_mapping *map, u16 len, const u8 *val)
   {
     return __tlv_format_str(dev, map, len, val) ? 0 : -ENOMEM;
   }

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    |