Re: [PATCH v4 15/17] module: Introduce hash-based integrity checking

["Thomas Weißschuh" <linux@weissschuh.net>]

On 2026-03-10 18:12:18-0700, Eric Biggers wrote:
> On Tue, Jan 13, 2026 at 01:28:59PM +0100, Thomas Weißschuh wrote:
> > The current signature-based module integrity checking has some drawbacks
> > in combination with reproducible builds. Either the module signing key
> > is generated at build time, which makes the build unreproducible, or a
> > static signing key is used, which precludes rebuilds by third parties
> > and makes the whole build and packaging process much more complicated.
> 
> I think this actually undersells the feature.

(...)

> So I think this is how module authentication should have been done
> originally, and I'm glad to see this is finally being fixed.

Thanks, that is nice to hear.

> > +struct module_hashes_proof {
> > +	__be32 pos;
> > +	u8 hash_sigs[][MODULE_HASHES_HASH_SIZE];
> > +} __packed;
> 
> Is the choice of big endian for consistency with struct
> module_signature?  Little endian is the usual choice in new code.

Yes, it's for consistency. But I am fine with either way. Given that
this is essentially an internal ABI, we could always change it later.

> > diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> > index a45ce3b24403..3b510651830d 100644
> > --- a/include/linux/module_signature.h
> > +++ b/include/linux/module_signature.h
> > @@ -18,6 +18,7 @@ enum pkey_id_type {
> >  	PKEY_ID_PGP,		/* OpenPGP generated key ID */
> >  	PKEY_ID_X509,		/* X.509 arbitrary subjectKeyIdentifier */
> >  	PKEY_ID_PKCS7,		/* Signature in PKCS#7 message */
> > +	PKEY_ID_MERKLE,		/* Merkle proof for modules */
> 
> I recommend making the hash algorithm explicit:
> 
>         PKEY_ID_MERKLE_SHA256,	/* SHA-256 merkle proof for modules */
> 
> While I wouldn't encourage the addition of another hash algorithm
> (specifying one good algorithm for now is absolutely the right choice),
> if someone ever does need to add another one, we'd want them to be
> guided to simply introduce a new value of this enum rather than hack it
> in some other way.

The idea here was that this will only ever be used for module built as
part of the kernel build. So the actual implementation could change freely
without affecting anything.

But I don't have hard feelings about it.

> > +static void hash_entry(const void *left, const void *right, void *out)
> 
> Byte arrays should use u8 instead of void

Ack.

> > diff --git a/scripts/modules-merkle-tree.c b/scripts/modules-merkle-tree.c
> [...]
> 
> > +struct file_entry {
> > +	char *name;
> > +	unsigned int pos;
> > +	unsigned char hash[EVP_MAX_MD_SIZE];
> 
> Considering that the hash algorithm is fixed, EVP_MAX_MD_SIZE can be
> replaced with a tighter local definition:

Ack.

>     #define MAX_HASH_SIZE 32

IMO it shouldn't even mention 'MAX', as there is only one hash
algorithm.

(...)

> > +{
> > +	fprintf(stderr,
> > +		"Usage: scripts/modules-merkle-tree <root definition>\n");
> > +	exit(2);
> 
> This should show both parameters, <root hash> <new suffix>

Ack.

> But they probably should be flipped to put the output second.

Ack.

> Though, is <new suffix> needed at all?  It looks like it doesn't
> actually affect the output.

It will be required for compatibility with INSTALL_MOD_STRIP,
two patches later. I'll move this code into the later patch.

> > +	hash_evp = EVP_get_digestbyname("sha256");
> 
> EVP_sha256()

(...)

Ack to all other remarks.


Thomas