The logic to extract the signature bits from a module file are
duplicated between the module core and IMA modsig appraisal.

Unify the implementation.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 include/linux/module_signature.h    |  4 +--
 kernel/module/signing.c             | 52 +++++++------------------------------
 kernel/module_signature.c           | 41 +++++++++++++++++++++++++++--
 security/integrity/ima/ima_modsig.c | 24 ++++-------------
 4 files changed, 56 insertions(+), 65 deletions(-)

diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
index 7eb4b00381ac..186a55effa30 100644
--- a/include/linux/module_signature.h
+++ b/include/linux/module_signature.h
@@ -40,7 +40,7 @@ struct module_signature {
 	__be32	sig_len;	/* Length of signature data */
 };
 
-int mod_check_sig(const struct module_signature *ms, size_t file_len,
-		  const char *name);
+int mod_split_sig(const void *buf, size_t *buf_len, bool mangled,
+		  size_t *sig_len, const u8 **sig, const char *name);
 
 #endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/kernel/module/signing.c b/kernel/module/signing.c
index fe3f51ac6199..6d64c0d18d0a 100644
--- a/kernel/module/signing.c
+++ b/kernel/module/signing.c
@@ -37,54 +37,22 @@ void set_module_sig_enforced(void)
 	sig_enforce = true;
 }
 
-/*
- * Verify the signature on a module.
- */
-static int mod_verify_sig(const void *mod, struct load_info *info)
-{
-	struct module_signature ms;
-	size_t sig_len, modlen = info->len;
-	int ret;
-
-	pr_devel("==>%s(,%zu)\n", __func__, modlen);
-
-	if (modlen <= sizeof(ms))
-		return -EBADMSG;
-
-	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
-
-	ret = mod_check_sig(&ms, modlen, "module");
-	if (ret)
-		return ret;
-
-	sig_len = be32_to_cpu(ms.sig_len);
-	modlen -= sig_len + sizeof(ms);
-	info->len = modlen;
-
-	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
-				      VERIFY_USE_SECONDARY_KEYRING,
-				      VERIFYING_MODULE_SIGNATURE,
-				      NULL, NULL);
-}
-
 int module_sig_check(struct load_info *info, int flags)
 {
-	int err = -ENODATA;
-	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+	int err;
 	const char *reason;
 	const void *mod = info->hdr;
+	size_t sig_len;
+	const u8 *sig;
 	bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
 				       MODULE_INIT_IGNORE_VERMAGIC);
-	/*
-	 * Do not allow mangled modules as a module with version information
-	 * removed is no longer the module that was signed.
-	 */
-	if (!mangled_module &&
-	    info->len > markerlen &&
-	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
-		/* We truncate the module to discard the signature */
-		info->len -= markerlen;
-		err = mod_verify_sig(mod, info);
+
+	err = mod_split_sig(info->hdr, &info->len, mangled_module, &sig_len, &sig, "module");
+	if (!err) {
+		err = verify_pkcs7_signature(mod, info->len, sig, sig_len,
+					     VERIFY_USE_SECONDARY_KEYRING,
+					     VERIFYING_MODULE_SIGNATURE,
+					     NULL, NULL);
 		if (!err) {
 			info->sig_ok = true;
 			return 0;
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
index 00132d12487c..b2384a73524c 100644
--- a/kernel/module_signature.c
+++ b/kernel/module_signature.c
@@ -8,6 +8,7 @@
 
 #include <linux/errno.h>
 #include <linux/printk.h>
+#include <linux/string.h>
 #include <linux/module_signature.h>
 #include <asm/byteorder.h>
 
@@ -18,8 +19,8 @@
  * @file_len:	Size of the file to which @ms is appended.
  * @name:	What is being checked. Used for error messages.
  */
-int mod_check_sig(const struct module_signature *ms, size_t file_len,
-		  const char *name)
+static int mod_check_sig(const struct module_signature *ms, size_t file_len,
+			 const char *name)
 {
 	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
 		return -EBADMSG;
@@ -44,3 +45,39 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
 
 	return 0;
 }
+
+int mod_split_sig(const void *buf, size_t *buf_len, bool mangled,
+		  size_t *sig_len, const u8 **sig, const char *name)
+{
+	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+	struct module_signature ms;
+	size_t modlen = *buf_len;
+	int ret;
+
+	/*
+	 * Do not allow mangled modules as a module with version information
+	 * removed is no longer the module that was signed.
+	 */
+	if (!mangled &&
+	    *buf_len > markerlen &&
+	    memcmp(buf + modlen - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
+		/* We truncate the module to discard the signature */
+		modlen -= markerlen;
+	}
+
+	if (modlen <= sizeof(ms))
+		return -EBADMSG;
+
+	memcpy(&ms, buf + (modlen - sizeof(ms)), sizeof(ms));
+
+	ret = mod_check_sig(&ms, modlen, name);
+	if (ret)
+		return ret;
+
+	*sig_len = be32_to_cpu(ms.sig_len);
+	modlen -= *sig_len + sizeof(ms);
+	*buf_len = modlen;
+	*sig = buf + modlen;
+
+	return 0;
+}
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index 3265d744d5ce..a57342d39b07 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -40,44 +40,30 @@ struct modsig {
 int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
 		    struct modsig **modsig)
 {
-	const size_t marker_len = strlen(MODULE_SIG_STRING);
-	const struct module_signature *sig;
+	size_t buf_len_sz = buf_len;
 	struct modsig *hdr;
 	size_t sig_len;
-	const void *p;
+	const u8 *sig;
 	int rc;
 
-	if (buf_len <= marker_len + sizeof(*sig))
-		return -ENOENT;
-
-	p = buf + buf_len - marker_len;
-	if (memcmp(p, MODULE_SIG_STRING, marker_len))
-		return -ENOENT;
-
-	buf_len -= marker_len;
-	sig = (const struct module_signature *)(p - sizeof(*sig));
-
-	rc = mod_check_sig(sig, buf_len, func_tokens[func]);
+	rc = mod_split_sig(buf, &buf_len_sz, true, &sig_len, &sig, func_tokens[func]);
 	if (rc)
 		return rc;
 
-	sig_len = be32_to_cpu(sig->sig_len);
-	buf_len -= sig_len + sizeof(*sig);
-
 	/* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
 	hdr = kzalloc(struct_size(hdr, raw_pkcs7, sig_len), GFP_KERNEL);
 	if (!hdr)
 		return -ENOMEM;
 
 	hdr->raw_pkcs7_len = sig_len;
-	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
+	hdr->pkcs7_msg = pkcs7_parse_message(sig, sig_len);
 	if (IS_ERR(hdr->pkcs7_msg)) {
 		rc = PTR_ERR(hdr->pkcs7_msg);
 		kfree(hdr);
 		return rc;
 	}
 
-	memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
+	memcpy(hdr->raw_pkcs7, sig, sig_len);
 
 	/* We don't know the hash algorithm yet. */
 	hdr->hash_algo = HASH_ALGO__LAST;

-- 
2.52.0

On 1/13/26 1:28 PM, Thomas Weißschuh wrote:
> The logic to extract the signature bits from a module file are
> duplicated between the module core and IMA modsig appraisal.
> 
> Unify the implementation.
> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
>  include/linux/module_signature.h    |  4 +--
>  kernel/module/signing.c             | 52 +++++++------------------------------
>  kernel/module_signature.c           | 41 +++++++++++++++++++++++++++--
>  security/integrity/ima/ima_modsig.c | 24 ++++-------------
>  4 files changed, 56 insertions(+), 65 deletions(-)
> 
> diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> index 7eb4b00381ac..186a55effa30 100644
> --- a/include/linux/module_signature.h
> +++ b/include/linux/module_signature.h
> @@ -40,7 +40,7 @@ struct module_signature {
>  	__be32	sig_len;	/* Length of signature data */
>  };
>  
> -int mod_check_sig(const struct module_signature *ms, size_t file_len,
> -		  const char *name);
> +int mod_split_sig(const void *buf, size_t *buf_len, bool mangled,
> +		  size_t *sig_len, const u8 **sig, const char *name);
>  
>  #endif /* _LINUX_MODULE_SIGNATURE_H */
> diff --git a/kernel/module/signing.c b/kernel/module/signing.c
> index fe3f51ac6199..6d64c0d18d0a 100644
> --- a/kernel/module/signing.c
> +++ b/kernel/module/signing.c
> @@ -37,54 +37,22 @@ void set_module_sig_enforced(void)
>  	sig_enforce = true;
>  }
>  
> -/*
> - * Verify the signature on a module.
> - */
> -static int mod_verify_sig(const void *mod, struct load_info *info)
> -{
> -	struct module_signature ms;
> -	size_t sig_len, modlen = info->len;
> -	int ret;
> -
> -	pr_devel("==>%s(,%zu)\n", __func__, modlen);
> -
> -	if (modlen <= sizeof(ms))
> -		return -EBADMSG;
> -
> -	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
> -
> -	ret = mod_check_sig(&ms, modlen, "module");
> -	if (ret)
> -		return ret;
> -
> -	sig_len = be32_to_cpu(ms.sig_len);
> -	modlen -= sig_len + sizeof(ms);
> -	info->len = modlen;
> -
> -	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
> -				      VERIFY_USE_SECONDARY_KEYRING,
> -				      VERIFYING_MODULE_SIGNATURE,
> -				      NULL, NULL);
> -}
> -
>  int module_sig_check(struct load_info *info, int flags)
>  {
> -	int err = -ENODATA;
> -	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> +	int err;
>  	const char *reason;
>  	const void *mod = info->hdr;
> +	size_t sig_len;
> +	const u8 *sig;
>  	bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
>  				       MODULE_INIT_IGNORE_VERMAGIC);
> -	/*
> -	 * Do not allow mangled modules as a module with version information
> -	 * removed is no longer the module that was signed.
> -	 */
> -	if (!mangled_module &&
> -	    info->len > markerlen &&
> -	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> -		/* We truncate the module to discard the signature */
> -		info->len -= markerlen;
> -		err = mod_verify_sig(mod, info);
> +
> +	err = mod_split_sig(info->hdr, &info->len, mangled_module, &sig_len, &sig, "module");
> +	if (!err) {
> +		err = verify_pkcs7_signature(mod, info->len, sig, sig_len,
> +					     VERIFY_USE_SECONDARY_KEYRING,
> +					     VERIFYING_MODULE_SIGNATURE,
> +					     NULL, NULL);
>  		if (!err) {
>  			info->sig_ok = true;
>  			return 0;

The patch looks to modify the behavior when mangled_module is true.

Previously, module_sig_check() didn't attempt to extract the signature
in such a case and treated the module as unsigned. The err remained set
to -ENODATA and the function subsequently consulted module_sig_check()
and security_locked_down() to determine an appropriate result.

Newly, module_sig_check() calls mod_split_sig(), which skips the
extraction of the marker ("~Module signature appended~\n") from the end
of the module and instead attempts to read it as an actual
module_signature. The value is then passed to mod_check_sig() which
should return -EBADMSG. The error is propagated to module_sig_check()
and treated as fatal, without consulting module_sig_check() and
security_locked_down().

I think the mangled_module flag should not be passed to mod_split_sig()
and it should be handled solely by module_sig_check().

> diff --git a/kernel/module_signature.c b/kernel/module_signature.c
> index 00132d12487c..b2384a73524c 100644
> --- a/kernel/module_signature.c
> +++ b/kernel/module_signature.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/errno.h>
>  #include <linux/printk.h>
> +#include <linux/string.h>
>  #include <linux/module_signature.h>
>  #include <asm/byteorder.h>
>  
> @@ -18,8 +19,8 @@
>   * @file_len:	Size of the file to which @ms is appended.
>   * @name:	What is being checked. Used for error messages.
>   */
> -int mod_check_sig(const struct module_signature *ms, size_t file_len,
> -		  const char *name)
> +static int mod_check_sig(const struct module_signature *ms, size_t file_len,
> +			 const char *name)
>  {
>  	if (be32_to_cpu(ms->sig_len) >= file_len - sizeof(*ms))
>  		return -EBADMSG;
> @@ -44,3 +45,39 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
>  
>  	return 0;
>  }
> +
> +int mod_split_sig(const void *buf, size_t *buf_len, bool mangled,
> +		  size_t *sig_len, const u8 **sig, const char *name)
> +{
> +	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> +	struct module_signature ms;
> +	size_t modlen = *buf_len;
> +	int ret;
> +
> +	/*
> +	 * Do not allow mangled modules as a module with version information
> +	 * removed is no longer the module that was signed.
> +	 */
> +	if (!mangled &&
> +	    *buf_len > markerlen &&
> +	    memcmp(buf + modlen - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> +		/* We truncate the module to discard the signature */
> +		modlen -= markerlen;
> +	}
> +
> +	if (modlen <= sizeof(ms))
> +		return -EBADMSG;
> +
> +	memcpy(&ms, buf + (modlen - sizeof(ms)), sizeof(ms));
> +
> +	ret = mod_check_sig(&ms, modlen, name);
> +	if (ret)
> +		return ret;
> +
> +	*sig_len = be32_to_cpu(ms.sig_len);
> +	modlen -= *sig_len + sizeof(ms);
> +	*buf_len = modlen;
> +	*sig = buf + modlen;
> +
> +	return 0;
> +}
> diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> index 3265d744d5ce..a57342d39b07 100644
> --- a/security/integrity/ima/ima_modsig.c
> +++ b/security/integrity/ima/ima_modsig.c
> @@ -40,44 +40,30 @@ struct modsig {
>  int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
>  		    struct modsig **modsig)
>  {
> -	const size_t marker_len = strlen(MODULE_SIG_STRING);
> -	const struct module_signature *sig;
> +	size_t buf_len_sz = buf_len;
>  	struct modsig *hdr;
>  	size_t sig_len;
> -	const void *p;
> +	const u8 *sig;
>  	int rc;
>  
> -	if (buf_len <= marker_len + sizeof(*sig))
> -		return -ENOENT;
> -
> -	p = buf + buf_len - marker_len;
> -	if (memcmp(p, MODULE_SIG_STRING, marker_len))
> -		return -ENOENT;
> -
> -	buf_len -= marker_len;
> -	sig = (const struct module_signature *)(p - sizeof(*sig));
> -
> -	rc = mod_check_sig(sig, buf_len, func_tokens[func]);
> +	rc = mod_split_sig(buf, &buf_len_sz, true, &sig_len, &sig, func_tokens[func]);

Passing mangled=true to mod_split_sig() seems incorrect here. It causes
that the function doesn't properly extract the signature marker at the
end of the module, no?

>  	if (rc)
>  		return rc;
>  
> -	sig_len = be32_to_cpu(sig->sig_len);
> -	buf_len -= sig_len + sizeof(*sig);
> -
>  	/* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
>  	hdr = kzalloc(struct_size(hdr, raw_pkcs7, sig_len), GFP_KERNEL);
>  	if (!hdr)
>  		return -ENOMEM;
>  
>  	hdr->raw_pkcs7_len = sig_len;
> -	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
> +	hdr->pkcs7_msg = pkcs7_parse_message(sig, sig_len);
>  	if (IS_ERR(hdr->pkcs7_msg)) {
>  		rc = PTR_ERR(hdr->pkcs7_msg);
>  		kfree(hdr);
>  		return rc;
>  	}
>  
> -	memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
> +	memcpy(hdr->raw_pkcs7, sig, sig_len);
>  
>  	/* We don't know the hash algorithm yet. */
>  	hdr->hash_algo = HASH_ALGO__LAST;
> 

-- 
Thanks,
Petr

On 2026-01-27 16:20:15+0100, Petr Pavlu wrote:
> On 1/13/26 1:28 PM, Thomas Weißschuh wrote:

(...)

> >  int module_sig_check(struct load_info *info, int flags)
> >  {
> > -	int err = -ENODATA;
> > -	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > +	int err;
> >  	const char *reason;
> >  	const void *mod = info->hdr;
> > +	size_t sig_len;
> > +	const u8 *sig;
> >  	bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
> >  				       MODULE_INIT_IGNORE_VERMAGIC);
> > -	/*
> > -	 * Do not allow mangled modules as a module with version information
> > -	 * removed is no longer the module that was signed.
> > -	 */
> > -	if (!mangled_module &&
> > -	    info->len > markerlen &&
> > -	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> > -		/* We truncate the module to discard the signature */
> > -		info->len -= markerlen;
> > -		err = mod_verify_sig(mod, info);
> > +
> > +	err = mod_split_sig(info->hdr, &info->len, mangled_module, &sig_len, &sig, "module");
> > +	if (!err) {
> > +		err = verify_pkcs7_signature(mod, info->len, sig, sig_len,
> > +					     VERIFY_USE_SECONDARY_KEYRING,
> > +					     VERIFYING_MODULE_SIGNATURE,
> > +					     NULL, NULL);
> >  		if (!err) {
> >  			info->sig_ok = true;
> >  			return 0;
> 
> The patch looks to modify the behavior when mangled_module is true.
> 
> Previously, module_sig_check() didn't attempt to extract the signature
> in such a case and treated the module as unsigned. The err remained set
> to -ENODATA and the function subsequently consulted module_sig_check()
> and security_locked_down() to determine an appropriate result.
> 
> Newly, module_sig_check() calls mod_split_sig(), which skips the
> extraction of the marker ("~Module signature appended~\n") from the end
> of the module and instead attempts to read it as an actual
> module_signature. The value is then passed to mod_check_sig() which
> should return -EBADMSG. The error is propagated to module_sig_check()
> and treated as fatal, without consulting module_sig_check() and
> security_locked_down().
> 
> I think the mangled_module flag should not be passed to mod_split_sig()
> and it should be handled solely by module_sig_check().

Ack.

(...)

> > diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> > index 3265d744d5ce..a57342d39b07 100644
> > --- a/security/integrity/ima/ima_modsig.c
> > +++ b/security/integrity/ima/ima_modsig.c
> > @@ -40,44 +40,30 @@ struct modsig {
> >  int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
> >  		    struct modsig **modsig)
> >  {
> > -	const size_t marker_len = strlen(MODULE_SIG_STRING);
> > -	const struct module_signature *sig;
> > +	size_t buf_len_sz = buf_len;
> >  	struct modsig *hdr;
> >  	size_t sig_len;
> > -	const void *p;
> > +	const u8 *sig;
> >  	int rc;
> >  
> > -	if (buf_len <= marker_len + sizeof(*sig))
> > -		return -ENOENT;
> > -
> > -	p = buf + buf_len - marker_len;
> > -	if (memcmp(p, MODULE_SIG_STRING, marker_len))
> > -		return -ENOENT;
> > -
> > -	buf_len -= marker_len;
> > -	sig = (const struct module_signature *)(p - sizeof(*sig));
> > -
> > -	rc = mod_check_sig(sig, buf_len, func_tokens[func]);
> > +	rc = mod_split_sig(buf, &buf_len_sz, true, &sig_len, &sig, func_tokens[func]);
> 
> Passing mangled=true to mod_split_sig() seems incorrect here. It causes
> that the function doesn't properly extract the signature marker at the
> end of the module, no?

Indeed, thanks.
 
I am thinking about dropping this patch from the series for now.
It was meant for IMA modsig compatibility, which is not part of the
series anymore.

> >  	if (rc)
> >  		return rc;
> >  
> > -	sig_len = be32_to_cpu(sig->sig_len);
> > -	buf_len -= sig_len + sizeof(*sig);
> > -
> >  	/* Allocate sig_len additional bytes to hold the raw PKCS#7 data. */
> >  	hdr = kzalloc(struct_size(hdr, raw_pkcs7, sig_len), GFP_KERNEL);
> >  	if (!hdr)
> >  		return -ENOMEM;
> >  
> >  	hdr->raw_pkcs7_len = sig_len;
> > -	hdr->pkcs7_msg = pkcs7_parse_message(buf + buf_len, sig_len);
> > +	hdr->pkcs7_msg = pkcs7_parse_message(sig, sig_len);
> >  	if (IS_ERR(hdr->pkcs7_msg)) {
> >  		rc = PTR_ERR(hdr->pkcs7_msg);
> >  		kfree(hdr);
> >  		return rc;
> >  	}
> >  
> > -	memcpy(hdr->raw_pkcs7, buf + buf_len, sig_len);
> > +	memcpy(hdr->raw_pkcs7, sig, sig_len);
> >  
> >  	/* We don't know the hash algorithm yet. */
> >  	hdr->hash_algo = HASH_ALGO__LAST;
> >