Re: [PATCH v2] tlv: Add tlv_bind_soc_uid mapping

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

On Tue, Nov 18, 2025 at 10:49:32AM +0100, Jonas Rebmann wrote:
> 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.

At least all drivers in the Kernel use either [0-9A-F] or [0-9].

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

The string you are referring here is allocated in __tlv_format_str():

const char * __tlv_format_str(struct tlv_device *dev, struct tlv_mapping *map, u16 len, const u8 *val)
{
	return __tlv_format(dev, map, basprintf("%.*s", len, val));
}

tlv_serial is another string. It's not freed and not even passes to tlv
functions.

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

Ok, I wrongly looked at tlv_format_str() which returns an int. So I
rephrase my question: Why not use tlv_format_str() instead?

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 |