Hi Yan,
On 2026-10-23T09:06:06, yan wang <[email protected]> wrote:
> binman: pre_load: 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.
The commit message is the same as in v1. Please can you describe the
hsm-key property, the pre-load-hsm-library / pre-load-hsm-pin entry
arguments and PKCS11_SIGN_PIN, and give the reason for the change,
i.e. so the private key never has to leave the token? Your cover
letter already says this well.
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -53,6 +82,37 @@ class Entry_pre_load(Entry_collection):
> + This entry creates a pre-load header that contains a global image
> + signature. The signature can be created using either a traditional RSA
> + key file (key-name) or a Hardware Security Module (HSM) via PKCS#11
> + (hsm-key), enabling production keys to remain on the HSM and never touch
> + the build machine's filesystem.
> +
>
> This entry creates a pre-load header that contains a global
> image signature.
The old paragraph is still there, so the docs now say this twice, with
two blank lines in between. Please merge them. Also, a key file can be
ECDSA as well, so 'RSA' is not right here.
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -87,6 +169,18 @@ class Entry_pre_load(Entry_collection):
> + self.hsm_library = fdt_util.GetString(self._node, 'hsm-library')
The cover letter says the library is not stored in the devicetree, but
the DT can provide it in three ways: this hsm-library property, a
'pre-load-hsm-library' property (see below) and module-path= in the
URI. Please can you drop the hsm-library property? The entry argument
and module-path= cover it.
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -94,6 +188,50 @@ class Entry_pre_load(Entry_collection):
> + # PIN from entry arg or environment variable, NOT from the DT.
> + pin_arg, = self.GetEntryArgsOrProps([EntryArg('pre-load-hsm-pin',
> str)])
> + self.hsm_pin = pin_arg or os.environ.get('PKCS11_SIGN_PIN')
One thing I'd like to check here: GetEntryArgsOrProps() calls
GetArg(), which falls back to fdt_util.GetDatatype() on the node when
no -a argument is given. So 'pre-load-hsm-pin = '1234';' in the DT is
still used, despite the comment. Please can you use
state.GetEntryArg('pre-load-hsm-pin') directly so the DT is never read
for the PIN? A test that puts the PIN in the DT and checks it is
ignored would be good.
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -94,6 +188,50 @@ class Entry_pre_load(Entry_collection):
> + hash_name, sign_name = self.algo_name.split(',')
> + if hash_name not in SHAS:
> + self.Raise(f'{hash_name} is not supported')
> + if sign_name not in RSAS and sign_name not in ECDSAS:
> + self.Raise(f'{sign_name} is not supported')
> + padding_name = self.padding_name or 'pkcs-1.5'
> + if padding_name not in ('pss', 'pkcs-1.5'):
> + self.Raise(f'Unsupported padding scheme: {padding_name}')
_SignHsmData() repeats these checks. Since ReadNode() always runs
first, please keep them here and work out the values once, e.g. in
instance variables.
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -197,8 +335,90 @@ class Entry_pre_load(Entry_collection):
> + When HSM signing is active the pkcs11-tool btool is called twice per
> + _CreateHeader() invocation (once for the image signature and once for
> + the header signature). Binman may call ProcessContents() several
> times
> + as entries settle, so consider caching the slot and mechanism results
> + in a real deployment against a rate-limited HSM.
Someone running binman can't act on this. Please either do the caching
or drop the paragraph (perhaps leave a short TODO instead).
> diff --git a/tools/binman/etype/pre_load.py b/tools/binman/etype/pre_load.py
> @@ -197,8 +335,90 @@ class Entry_pre_load(Entry_collection):
> + hash_sig = SHA256.new(img_sig)
> +
> + version = self.version
> + header_size = self.header_size
> + image_size = len(self.image)
> + ofs_img_sig = 64 + len(img_sig)
> + flags = 0
> + reserved0 = 0
> + reserved1 = 0
> +
> + first_header = struct.pack('>4sIIIIIII32s', PRE_LOAD_MAGIC,
> + version, header_size, image_size,
> + ofs_img_sig, flags, reserved0,
> + reserved1, hash_sig.digest())
This is the third copy of the header-building code, after
_CreateHeaderRsa() and _CreateHeaderEcdsa(). It would be worth a
preparatory patch with a helper that takes a signing function and
builds the header, so the HSM path only has to provide _SignHsmData().
What do you think?
>
> Signed-off-by: yan wang <[email protected]>
>
> tools/binman/etype/pre_load.py | 222
> ++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 221 insertions(+), 1 deletion(-)
Regards,
Simon
