Hi Yan, On 2026-10-12T10:47:59, Yan WANG <[email protected]> wrote: > binman: Add HSM signing support to pre-load entry type > > Extend the pre-load entry type to support signing via hardware > security modules (HSM) using PKCS#11. The implementation integrates > with the pkcs11_tool btool to perform signing operations.
This patch does much more than the commit message says. It: - moves the signing code into a new _SignData() helper - changes / to // in RSAS - rewrites the error messages - converts strings to f-strings - reformats the struct.pack() call - drops the image2 wrapper from the docstring example Please can you put the refactoring (extracting _SignData() with no functional change) in its own patch ahead of this one, so this patch only adds the HSM path? Please also list the new properties in the commit message and say why someone would want this, e.g. so that production keys never have to sit on the build machine. > > Signed-off-by: Yan WANG <[email protected]> > > tools/binman/etype/pre_load.py | 285 +++++++++++++++++++++++++++++++---------- > 1 file changed, 214 insertions(+), 71 deletions(-) > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -46,29 +47,57 @@ class Entry_pre_load(Entry_collection): > + HSM Signing Properties (optional): > + - hsm-support: Enable HSM signing (boolean) > + - hsm-library: Path to PKCS#11 library (.so file) > + - hsm-token-label: HSM token label > + - hsm-token-serial: HSM token serial number (alternative to > token-label) > + - hsm-key-label: HSM private key label > + - hsm-key-id: HSM private key ID (alternative to key-label) > + - hsm-pin: HSM user PIN The code also reads hsm-slot-index, but it isn't documented here. Please add it, and say which of label/serial/slot takes priority when more than one is given. Is hsm-support needed at all? The HSM path could be selected whenever hsm-library is present. That would also remove the 'hsm-library is required' error case. > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -79,6 +108,19 @@ class Entry_pre_load(Entry_collection): > + self.hsm_slot_index = fdt_util.GetString(self._node, > 'hsm-slot-index') A slot index is naturally an integer. With 'hsm-slot-index = <0>;', GetString() returns something strange rather than giving a clear error. I suggest using GetInt() and converting with str() when building the pkcs11-tool arguments. > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -79,6 +108,19 @@ class Entry_pre_load(Entry_collection): > + self.hsm_pin = fdt_util.GetString(self._node, 'hsm-pin') My main concern with the design is the PIN in the devicetree. The binman description normally lives in the U-Boot source tree (the -u-boot.dtsi files), so this pushes people towards committing a token PIN to git. The PIN also ends up in the binman output DTB and the fdtmap (if present). FIT signing with PKCS#11 already avoids this: mkimage reads the PIN from the MKIMAGE_SIGN_PIN environment variable, as used by testFitSignPKCS11Simple and friends in ftest.py. Please can you take the PIN from an entry argument (e.g. -a pre-load-hsm-pin=...) or the environment, like pre-load-key-path? Ideally the PIN should not be accepted as a DT property at all. > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -86,68 +128,169 @@ class Entry_pre_load(Entry_collection): > + if self.hsm_support: > + # HSM-based signing via pkcs11-tool btool > + if not self.pkcs11_tool.is_present(): > + self.Raise("pkcs11-tool btool not available for HSM signing") Other etypes (mkimage, x509_cert, fit, etc.) call self.record_missing_bintool() here. That way --allow-missing / --fake-ext-blobs keeps working and binman reports the missing tool in the usual way. Please follow that pattern: record the tool as missing and write a dummy header rather than raising. > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -86,68 +128,169 @@ class Entry_pre_load(Entry_collection): > + # Use slot index if provided (simpler than serial lookup) > + try: > + signature = self.pkcs11_tool.sign( The comment doesn't match the code. The code passes all the identifiers through and leaves the choice to sign(). More importantly, the HSM path skips the hash_name/sign_name checks against SHAS and RSAS, and doesn't check the size of the returned signature. U-Boot's image_pre_load_sig_check_header_sig() and image_pre_load_sig_check_img_sig_sha256() read info->sig_size bytes, which come from the verifying key, not the length binman used. So if algo-name says rsa2048 but the HSM key is 4096 bits, binman happily produces a header that can never verify. Equally, 'sha256,ec256' gets through to _build_mechanism() and produces an ECDSA signature, which this entry doesn't otherwise support. Please do the SHAS/RSAS checks for both paths, before the if, and check that len(signature) == RSAS[sign_name] after signing. > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -86,68 +128,169 @@ class Entry_pre_load(Entry_collection): > + # Sign the image data > + img_sig = self._SignData(self.image) > + > + # Compute hash of image signature (always SHA256 for header) > + hash_sig = SHA256.new(img_sig) On the HSM path, each _SignData() call runs pkcs11-tool three times (--list-slots, --list-mechanisms, --sign), and we sign twice per header. Binman can also call _CreateHeader() several times as entries settle (ObtainContents() and ProcessContents()). That adds up to quite a few logins to a real HSM, which may be slow or rate-limited. Not a blocker, but it might be worth resolving the slot and checking the mechanism once, then caching the result. What do you think? > diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py > @@ -86,68 +128,169 @@ class Entry_pre_load(Entry_collection): > + # Pack first header (64 bytes, big-endian) > + first_header = struct.pack( > + '>4sIIIIIII32s', > + PRE_LOAD_MAGIC, # 4 bytes: magic number 'UBSH' > + version, # 4 bytes: header version > + header_size, # 4 bytes: total header size > + image_size, # 4 bytes: image size > + ofs_img_sig, # 4 bytes: offset to image signature > + flags, # 4 bytes: flags (reserved) > + reserved0, # 4 bytes: reserved > + reserved1, # 4 bytes: reserved > + hash_sig.digest() # 32 bytes: SHA256 hash of image signature > + ) This only reformats the existing code. Please drop it, or move it into the refactoring patch. Regards, Simon
