[PATCH v2 0/6] KEXEC_SIG with appended signature

[PATCH v2 0/6] KEXEC_SIG with appended signature

[Michal Suchanek]

Hello,

This is resend of the KEXEC_SIG patchset.

The first patch is new because it'a a cleanup that does not require any
change to the module verification code.

The second patch is the only one that is intended to change any
functionality.

The rest only deduplicates code but I did not receive any review on that
part so I don't know if it's desirable as implemented.

The first two patches can be applied separately without the rest.

Thanks

Michal

Michal Suchanek (6):
  s390/kexec_file: Don't opencode appended signature check.
  powerpc/kexec_file: Add KEXEC_SIG support.
  kexec_file: Don't opencode appended signature verification.
  module: strip the signature marker in the verification function.
  module: Use key_being_used_for for log messages in
    verify_appended_signature
  module: Move duplicate mod_check_sig users code to mod_parse_sig

 arch/powerpc/Kconfig                     | 11 +++++
 arch/powerpc/kexec/elf_64.c              | 14 ++++++
 arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 include/linux/module_signature.h         |  1 +
 include/linux/verification.h             |  4 ++
 kernel/module-internal.h                 |  2 -
 kernel/module.c                          | 12 +++--
 kernel/module_signature.c                | 56 +++++++++++++++++++++++-
 kernel/module_signing.c                  | 33 +++++++-------
 security/integrity/ima/ima_modsig.c      | 22 ++--------
 11 files changed, 113 insertions(+), 85 deletions(-)

-- 
2.31.1

[PATCH v2 1/6] s390/kexec_file: Don't opencode appended signature check.

[Michal Suchanek]

Module verification already implements appeded signature check.

Reuse it for kexec_file.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/s390/kernel/machine_kexec_file.c | 22 +++++-----------------
 1 file changed, 5 insertions(+), 17 deletions(-)

diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index 9975ad200d74..43a9abe48abd 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -29,6 +29,7 @@ int s390_verify_sig(const char *kernel, unsigned long kernel_len)
 	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
 	struct module_signature *ms;
 	unsigned long sig_len;
+	int ret;
 
 	/* Skip signature verification when not secure IPLed. */
 	if (!ipl_secure_flag)
@@ -43,25 +44,12 @@ int s390_verify_sig(const char *kernel, unsigned long kernel_len)
 	kernel_len -= marker_len;
 
 	ms = (void *)kernel + kernel_len - sizeof(*ms);
-	kernel_len -= sizeof(*ms);
+	ret = mod_check_sig(ms, kernel_len, "kexec");
+	if (ret)
+		return ret;
 
 	sig_len = be32_to_cpu(ms->sig_len);
-	if (sig_len >= kernel_len)
-		return -EKEYREJECTED;
-	kernel_len -= sig_len;
-
-	if (ms->id_type != PKEY_ID_PKCS7)
-		return -EKEYREJECTED;
-
-	if (ms->algo != 0 ||
-	    ms->hash != 0 ||
-	    ms->signer_len != 0 ||
-	    ms->key_id_len != 0 ||
-	    ms->__pad[0] != 0 ||
-	    ms->__pad[1] != 0 ||
-	    ms->__pad[2] != 0) {
-		return -EBADMSG;
-	}
+	kernel_len -= sizeof(*ms) + sig_len;
 
 	return verify_pkcs7_signature(kernel, kernel_len,
 				      kernel + kernel_len, sig_len,
-- 
2.31.1

[PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

[Michal Suchanek]

Copy the code from s390x

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/Kconfig        | 11 +++++++++++
 arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index ac0c515552fd..ecc1227a77f1 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -561,6 +561,17 @@ config KEXEC_FILE
 config ARCH_HAS_KEXEC_PURGATORY
 	def_bool KEXEC_FILE
 
+config KEXEC_SIG
+	bool "Verify kernel signature during kexec_file_load() syscall"
+	depends on KEXEC_FILE && MODULE_SIG_FORMAT
+	help
+	  This option makes kernel signature verification mandatory for
+	  the kexec_file_load() syscall.
+
+	  In addition to that option, you need to enable signature
+	  verification for the corresponding kernel image type being
+	  loaded in order for this to work.
+
 config PPC64_BUILD_ELF_V2_ABI
 	bool
 
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index eeb258002d1e..25dc1071feec 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -23,6 +23,7 @@
 #include <linux/of_fdt.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/verification.h>
 
 static void *elf64_load(struct kimage *image, char *kernel_buf,
 			unsigned long kernel_len, char *initrd,
@@ -151,7 +152,42 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 	return ret ? ERR_PTR(ret) : NULL;
 }
 
+#ifdef CONFIG_KEXEC_SIG
+int elf64_verify_sig(const char *kernel, unsigned long kernel_len)
+{
+	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
+	struct module_signature *ms;
+	unsigned long sig_len;
+	int ret;
+
+	if (marker_len > kernel_len)
+		return -EKEYREJECTED;
+
+	if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
+		   marker_len))
+		return -EKEYREJECTED;
+	kernel_len -= marker_len;
+
+	ms = (void *)kernel + kernel_len - sizeof(*ms);
+	ret = mod_check_sig(ms, kernel_len, "kexec");
+	if (ret)
+		return ret;
+
+	sig_len = be32_to_cpu(ms->sig_len);
+	kernel_len -= sizeof(*ms) + sig_len;
+
+	return verify_pkcs7_signature(kernel, kernel_len,
+				      kernel + kernel_len, sig_len,
+				      VERIFY_USE_PLATFORM_KEYRING,
+				      VERIFYING_MODULE_SIGNATURE,
+				      NULL, NULL);
+}
+#endif /* CONFIG_KEXEC_SIG */
+
 const struct kexec_file_ops kexec_elf64_ops = {
 	.probe = kexec_elf_probe,
 	.load = elf64_load,
+#ifdef CONFIG_KEXEC_SIG
+	.verify_sig = elf64_verify_sig,
+#endif
 };
-- 
2.31.1

Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

[Michal Suchánek]

Hello,

On Wed, Dec 08, 2021 at 08:51:47PM -0500, Nayna wrote:
> 
> On 11/25/21 13:02, Michal Suchanek wrote:
> > Copy the code from s390x
> > 
> > Signed-off-by: Michal Suchanek<msuchanek@suse.de>
> > ---
> >   arch/powerpc/Kconfig        | 11 +++++++++++
> >   arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 47 insertions(+)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index ac0c515552fd..ecc1227a77f1 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -561,6 +561,17 @@ config KEXEC_FILE
> >   config ARCH_HAS_KEXEC_PURGATORY
> >   	def_bool KEXEC_FILE
> > 
> > +config KEXEC_SIG
> > +	bool "Verify kernel signature during kexec_file_load() syscall"
> > +	depends on KEXEC_FILE && MODULE_SIG_FORMAT
> 
> After manually applying the patch, the build is failing with the following
> error:
> 
> build failed with error "arch/powerpc/kexec/elf_64.o: In function
> `elf64_verify_sig':
> /root/kernel/linus/linux/arch/powerpc/kexec/elf_64.c:160: undefined
> reference to `verify_appended_signature'"

This patch does not add call to verify_appended_signature.

Maybe you applied the following patch as well?

Thanks

Michal

Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

[Nayna]

On 12/9/21 04:21, Michal Suchánek wrote:
> Hello,
Hi,
> On Wed, Dec 08, 2021 at 08:51:47PM -0500, Nayna wrote:
>> On 11/25/21 13:02, Michal Suchanek wrote:
>>> Copy the code from s390x
>>>
>>> Signed-off-by: Michal Suchanek<msuchanek@suse.de>
>>> ---
>>>    arch/powerpc/Kconfig        | 11 +++++++++++
>>>    arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
>>>    2 files changed, 47 insertions(+)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index ac0c515552fd..ecc1227a77f1 100644
>>> --- a/arch/powerpc/Kconfig
>>> +++ b/arch/powerpc/Kconfig
>>> @@ -561,6 +561,17 @@ config KEXEC_FILE
>>>    config ARCH_HAS_KEXEC_PURGATORY
>>>    	def_bool KEXEC_FILE
>>>
>>> +config KEXEC_SIG
>>> +	bool "Verify kernel signature during kexec_file_load() syscall"
>>> +	depends on KEXEC_FILE && MODULE_SIG_FORMAT
>> After manually applying the patch, the build is failing with the following
>> error:
>>
>> build failed with error "arch/powerpc/kexec/elf_64.o: In function
>> `elf64_verify_sig':
>> /root/kernel/linus/linux/arch/powerpc/kexec/elf_64.c:160: undefined
>> reference to `verify_appended_signature'"
> This patch does not add call to verify_appended_signature.
>
> Maybe you applied the following patch as well?

Yes, I tried build after applying all the patches.

Thanks & Regards,

     - Nayna

Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

[Nayna]

On 11/25/21 13:02, Michal Suchanek wrote:
> Copy the code from s390x
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>   arch/powerpc/Kconfig        | 11 +++++++++++
>   arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 47 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index ac0c515552fd..ecc1227a77f1 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -561,6 +561,17 @@ config KEXEC_FILE
>   config ARCH_HAS_KEXEC_PURGATORY
>   	def_bool KEXEC_FILE
>
> +config KEXEC_SIG
> +	bool "Verify kernel signature during kexec_file_load() syscall"
> +	depends on KEXEC_FILE && MODULE_SIG_FORMAT
> +	help
> +	  This option makes kernel signature verification mandatory for
> +	  the kexec_file_load() syscall.
> +

Resending my last response as looks like it didn't go through mailing 
list because of some wrong formatting. My apologies to those who are 
receiving it twice.

Since powerpc also supports IMA_ARCH_POLICY for kernel image signature 
verification, please include the following:

"An alternative implementation for the powerpc arch is IMA_ARCH_POLICY. 
It verifies the appended kernel image signature and additionally 
includes both the signed and unsigned file hashes in the IMA measurement 
list, extends the IMA PCR in the TPM, and prevents blacklisted binary 
kernel images from being kexec'd."

Thanks & Regards,

     - Nayna

Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.

[Michal Suchánek]

Hello,

On Sun, Dec 12, 2021 at 07:46:53PM -0500, Nayna wrote:
> 
> On 11/25/21 13:02, Michal Suchanek wrote:
> > Copy the code from s390x
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >   arch/powerpc/Kconfig        | 11 +++++++++++
> >   arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
> >   2 files changed, 47 insertions(+)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index ac0c515552fd..ecc1227a77f1 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -561,6 +561,17 @@ config KEXEC_FILE
> >   config ARCH_HAS_KEXEC_PURGATORY
> >   	def_bool KEXEC_FILE
> > 
> > +config KEXEC_SIG
> > +	bool "Verify kernel signature during kexec_file_load() syscall"
> > +	depends on KEXEC_FILE && MODULE_SIG_FORMAT
> > +	help
> > +	  This option makes kernel signature verification mandatory for
> > +	  the kexec_file_load() syscall.
> > +
> 
> Resending my last response as looks like it didn't go through mailing list
> because of some wrong formatting. My apologies to those who are receiving it
> twice.
> 
> Since powerpc also supports IMA_ARCH_POLICY for kernel image signature
> verification, please include the following:
> 
> "An alternative implementation for the powerpc arch is IMA_ARCH_POLICY. It
> verifies the appended kernel image signature and additionally includes both
> the signed and unsigned file hashes in the IMA measurement list, extends the
> IMA PCR in the TPM, and prevents blacklisted binary kernel images from being
> kexec'd."

It also does blacklist based on the file hash?

There is a downstream patch that adds the support for the module
signatures, and when the code is reused for KEXEC_SIG the blacklist
also applies to it.

Which kind of shows that people really want to use the IMA features but
with no support on some major architectures it's not going to work.

Thanks

Michal

[PATCH v2 3/6] kexec_file: Don't opencode appended signature verification.

[Michal Suchanek]

Module verification already implements appeded signature verification.

Reuse it for kexec_file.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kexec/elf_64.c           | 21 ++++-----------------
 arch/s390/kernel/machine_kexec_file.c | 21 ++++-----------------
 include/linux/verification.h          |  3 +++
 kernel/module-internal.h              |  2 --
 kernel/module.c                       |  4 +++-
 kernel/module_signing.c               | 24 +++++++++++++++---------
 6 files changed, 29 insertions(+), 46 deletions(-)

diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 25dc1071feec..266cb26d3ca0 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -153,12 +153,10 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 }
 
 #ifdef CONFIG_KEXEC_SIG
-int elf64_verify_sig(const char *kernel, unsigned long kernel_len)
+int elf64_verify_sig(const char *kernel, unsigned long length)
 {
+	size_t kernel_len = length;
 	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
-	struct module_signature *ms;
-	unsigned long sig_len;
-	int ret;
 
 	if (marker_len > kernel_len)
 		return -EKEYREJECTED;
@@ -168,19 +166,8 @@ int elf64_verify_sig(const char *kernel, unsigned long kernel_len)
 		return -EKEYREJECTED;
 	kernel_len -= marker_len;
 
-	ms = (void *)kernel + kernel_len - sizeof(*ms);
-	ret = mod_check_sig(ms, kernel_len, "kexec");
-	if (ret)
-		return ret;
-
-	sig_len = be32_to_cpu(ms->sig_len);
-	kernel_len -= sizeof(*ms) + sig_len;
-
-	return verify_pkcs7_signature(kernel, kernel_len,
-				      kernel + kernel_len, sig_len,
-				      VERIFY_USE_PLATFORM_KEYRING,
-				      VERIFYING_MODULE_SIGNATURE,
-				      NULL, NULL);
+	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
+					"kexec_file");
 }
 #endif /* CONFIG_KEXEC_SIG */
 
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index 43a9abe48abd..432797249db3 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -24,12 +24,10 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
 };
 
 #ifdef CONFIG_KEXEC_SIG
-int s390_verify_sig(const char *kernel, unsigned long kernel_len)
+int s390_verify_sig(const char *kernel, unsigned long length)
 {
+	size_t kernel_len = length;
 	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
-	struct module_signature *ms;
-	unsigned long sig_len;
-	int ret;
 
 	/* Skip signature verification when not secure IPLed. */
 	if (!ipl_secure_flag)
@@ -43,19 +41,8 @@ int s390_verify_sig(const char *kernel, unsigned long kernel_len)
 		return -EKEYREJECTED;
 	kernel_len -= marker_len;
 
-	ms = (void *)kernel + kernel_len - sizeof(*ms);
-	ret = mod_check_sig(ms, kernel_len, "kexec");
-	if (ret)
-		return ret;
-
-	sig_len = be32_to_cpu(ms->sig_len);
-	kernel_len -= sizeof(*ms) + sig_len;
-
-	return verify_pkcs7_signature(kernel, kernel_len,
-				      kernel + kernel_len, sig_len,
-				      VERIFY_USE_PLATFORM_KEYRING,
-				      VERIFYING_MODULE_SIGNATURE,
-				      NULL, NULL);
+	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
+					"kexec_file");
 }
 #endif /* CONFIG_KEXEC_SIG */
 
diff --git a/include/linux/verification.h b/include/linux/verification.h
index a655923335ae..c1cf0582012a 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -60,5 +60,8 @@ extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
 				   enum key_being_used_for usage);
 #endif
 
+int verify_appended_signature(const void *data, size_t *len, struct key *trusted_keys,
+			      const char *what);
+
 #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 #endif /* _LINUX_VERIFY_PEFILE_H */
diff --git a/kernel/module-internal.h b/kernel/module-internal.h
index 33783abc377b..80461e14bf29 100644
--- a/kernel/module-internal.h
+++ b/kernel/module-internal.h
@@ -27,5 +27,3 @@ struct load_info {
 		unsigned int sym, str, mod, vers, info, pcpu;
 	} index;
 };
-
-extern int mod_verify_sig(const void *mod, struct load_info *info);
diff --git a/kernel/module.c b/kernel/module.c
index 84a9141a5e15..8481933dfa92 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -57,6 +57,7 @@
 #include <linux/bsearch.h>
 #include <linux/dynamic_debug.h>
 #include <linux/audit.h>
+#include <linux/verification.h>
 #include <uapi/linux/module.h>
 #include "module-internal.h"
 
@@ -2894,7 +2895,8 @@ static int module_sig_check(struct load_info *info, int flags)
 	    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 = verify_appended_signature(mod, &info->len,
+						VERIFY_USE_SECONDARY_KEYRING, "module");
 		if (!err) {
 			info->sig_ok = true;
 			return 0;
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 8723ae70ea1f..f492e410564d 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -14,13 +14,19 @@
 #include <crypto/public_key.h>
 #include "module-internal.h"
 
-/*
- * Verify the signature on a module.
+/**
+ * verify_appended_signature - Verify the signature on a module with the
+ * signature marker stripped.
+ * @data: The data to be verified
+ * @len: Size of @data.
+ * @trusted_keys: Keyring to use for verification
+ * @what: Informational string for log messages
  */
-int mod_verify_sig(const void *mod, struct load_info *info)
+int verify_appended_signature(const void *data, size_t *len,
+			      struct key *trusted_keys, const char *what)
 {
 	struct module_signature ms;
-	size_t sig_len, modlen = info->len;
+	size_t sig_len, modlen = *len;
 	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
@@ -28,18 +34,18 @@ int mod_verify_sig(const void *mod, struct load_info *info)
 	if (modlen <= sizeof(ms))
 		return -EBADMSG;
 
-	memcpy(&ms, mod + (modlen - sizeof(ms)), sizeof(ms));
+	memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
 
-	ret = mod_check_sig(&ms, modlen, "module");
+	ret = mod_check_sig(&ms, modlen, what);
 	if (ret)
 		return ret;
 
 	sig_len = be32_to_cpu(ms.sig_len);
 	modlen -= sig_len + sizeof(ms);
-	info->len = modlen;
+	*len = modlen;
 
-	return verify_pkcs7_signature(mod, modlen, mod + modlen, sig_len,
-				      VERIFY_USE_SECONDARY_KEYRING,
+	return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
+				      trusted_keys,
 				      VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);
 }
-- 
2.31.1

[PATCH v2 4/6] module: strip the signature marker in the verification function.

[Michal Suchanek]

It is stripped by each caller separately.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kexec/elf_64.c           |  9 ---------
 arch/s390/kernel/machine_kexec_file.c |  9 ---------
 kernel/module.c                       |  7 +------
 kernel/module_signing.c               | 12 ++++++++++--
 4 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 266cb26d3ca0..63634c95265d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -156,15 +156,6 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
 int elf64_verify_sig(const char *kernel, unsigned long length)
 {
 	size_t kernel_len = length;
-	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
-
-	if (marker_len > kernel_len)
-		return -EKEYREJECTED;
-
-	if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
-		   marker_len))
-		return -EKEYREJECTED;
-	kernel_len -= marker_len;
 
 	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
 					"kexec_file");
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index 432797249db3..c4632c1a1b59 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -27,20 +27,11 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
 int s390_verify_sig(const char *kernel, unsigned long length)
 {
 	size_t kernel_len = length;
-	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
 
 	/* Skip signature verification when not secure IPLed. */
 	if (!ipl_secure_flag)
 		return 0;
 
-	if (marker_len > kernel_len)
-		return -EKEYREJECTED;
-
-	if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
-		   marker_len))
-		return -EKEYREJECTED;
-	kernel_len -= marker_len;
-
 	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
 					"kexec_file");
 }
diff --git a/kernel/module.c b/kernel/module.c
index 8481933dfa92..d91ca0f93a40 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2882,7 +2882,6 @@ static inline void kmemleak_load_module(const struct module *mod,
 static int module_sig_check(struct load_info *info, int flags)
 {
 	int err = -ENODATA;
-	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
 	const char *reason;
 	const void *mod = info->hdr;
 
@@ -2890,11 +2889,7 @@ static int module_sig_check(struct load_info *info, int flags)
 	 * Require flags == 0, as a module with version information
 	 * removed is no longer the module that was signed
 	 */
-	if (flags == 0 &&
-	    info->len > markerlen &&
-	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
-		/* We truncate the module to discard the signature */
-		info->len -= markerlen;
+	if (flags == 0) {
 		err = verify_appended_signature(mod, &info->len,
 						VERIFY_USE_SECONDARY_KEYRING, "module");
 		if (!err) {
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index f492e410564d..4c28cb55275f 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -15,8 +15,7 @@
 #include "module-internal.h"
 
 /**
- * verify_appended_signature - Verify the signature on a module with the
- * signature marker stripped.
+ * verify_appended_signature - Verify the signature on a module
  * @data: The data to be verified
  * @len: Size of @data.
  * @trusted_keys: Keyring to use for verification
@@ -25,12 +24,21 @@
 int verify_appended_signature(const void *data, size_t *len,
 			      struct key *trusted_keys, const char *what)
 {
+	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
 	struct module_signature ms;
 	size_t sig_len, modlen = *len;
 	int ret;
 
 	pr_devel("==>%s(,%zu)\n", __func__, modlen);
 
+	if (markerlen > modlen)
+		return -ENODATA;
+
+	if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
+		   markerlen))
+		return -ENODATA;
+	modlen -= markerlen;
+
 	if (modlen <= sizeof(ms))
 		return -EBADMSG;
 
-- 
2.31.1

Re: [PATCH v2 4/6] module: strip the signature marker in the verification function.

[Philipp Rudo]

Hi Michal,

On Thu, 25 Nov 2021 19:02:42 +0100
Michal Suchanek <msuchanek@suse.de> wrote:

> It is stripped by each caller separately.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  arch/powerpc/kexec/elf_64.c           |  9 ---------
>  arch/s390/kernel/machine_kexec_file.c |  9 ---------
>  kernel/module.c                       |  7 +------
>  kernel/module_signing.c               | 12 ++++++++++--

kernel/module_signing.c is only compiled with MODULE_SIG enabled but
KEXEC_SIG only selects MODULE_SIG_FORMAT. In the unlikely case that
KEXEC_SIG is enabled but MODULE_SIG isn't this causes a build breakage.
So you need to update KEXEC_SIG to select MODULE_SIG instead of
MODULE_SIG_FORMAT for s390 and ppc.

Thanks
Philipp

>  4 files changed, 11 insertions(+), 26 deletions(-)
> 
> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
> index 266cb26d3ca0..63634c95265d 100644
> --- a/arch/powerpc/kexec/elf_64.c
> +++ b/arch/powerpc/kexec/elf_64.c
> @@ -156,15 +156,6 @@ static void *elf64_load(struct kimage *image, char *kernel_buf,
>  int elf64_verify_sig(const char *kernel, unsigned long length)
>  {
>  	size_t kernel_len = length;
> -	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
> -
> -	if (marker_len > kernel_len)
> -		return -EKEYREJECTED;
> -
> -	if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
> -		   marker_len))
> -		return -EKEYREJECTED;
> -	kernel_len -= marker_len;
>  
>  	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
>  					"kexec_file");
> diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
> index 432797249db3..c4632c1a1b59 100644
> --- a/arch/s390/kernel/machine_kexec_file.c
> +++ b/arch/s390/kernel/machine_kexec_file.c
> @@ -27,20 +27,11 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
>  int s390_verify_sig(const char *kernel, unsigned long length)
>  {
>  	size_t kernel_len = length;
> -	const unsigned long marker_len = sizeof(MODULE_SIG_STRING) - 1;
>  
>  	/* Skip signature verification when not secure IPLed. */
>  	if (!ipl_secure_flag)
>  		return 0;
>  
> -	if (marker_len > kernel_len)
> -		return -EKEYREJECTED;
> -
> -	if (memcmp(kernel + kernel_len - marker_len, MODULE_SIG_STRING,
> -		   marker_len))
> -		return -EKEYREJECTED;
> -	kernel_len -= marker_len;
> -
>  	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
>  					"kexec_file");
>  }
> diff --git a/kernel/module.c b/kernel/module.c
> index 8481933dfa92..d91ca0f93a40 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2882,7 +2882,6 @@ static inline void kmemleak_load_module(const struct module *mod,
>  static int module_sig_check(struct load_info *info, int flags)
>  {
>  	int err = -ENODATA;
> -	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
>  	const char *reason;
>  	const void *mod = info->hdr;
>  
> @@ -2890,11 +2889,7 @@ static int module_sig_check(struct load_info *info, int flags)
>  	 * Require flags == 0, as a module with version information
>  	 * removed is no longer the module that was signed
>  	 */
> -	if (flags == 0 &&
> -	    info->len > markerlen &&
> -	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
> -		/* We truncate the module to discard the signature */
> -		info->len -= markerlen;
> +	if (flags == 0) {
>  		err = verify_appended_signature(mod, &info->len,
>  						VERIFY_USE_SECONDARY_KEYRING, "module");
>  		if (!err) {
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index f492e410564d..4c28cb55275f 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -15,8 +15,7 @@
>  #include "module-internal.h"
>  
>  /**
> - * verify_appended_signature - Verify the signature on a module with the
> - * signature marker stripped.
> + * verify_appended_signature - Verify the signature on a module
>   * @data: The data to be verified
>   * @len: Size of @data.
>   * @trusted_keys: Keyring to use for verification
> @@ -25,12 +24,21 @@
>  int verify_appended_signature(const void *data, size_t *len,
>  			      struct key *trusted_keys, const char *what)
>  {
> +	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
>  	struct module_signature ms;
>  	size_t sig_len, modlen = *len;
>  	int ret;
>  
>  	pr_devel("==>%s(,%zu)\n", __func__, modlen);  
>  
> +	if (markerlen > modlen)
> +		return -ENODATA;
> +
> +	if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> +		   markerlen))
> +		return -ENODATA;
> +	modlen -= markerlen;
> +
>  	if (modlen <= sizeof(ms))
>  		return -EBADMSG;
>  

[PATCH v2 5/6] module: Use key_being_used_for for log messages in verify_appended_signature

[Michal Suchanek]

Add value for kexec appended signature and pass in key_being_used_for
enum rather than a string to verify_appended_signature to produce log
messages about the signature.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 arch/powerpc/kexec/elf_64.c              |  2 +-
 arch/s390/kernel/machine_kexec_file.c    |  2 +-
 crypto/asymmetric_keys/asymmetric_type.c |  1 +
 include/linux/verification.h             |  3 ++-
 kernel/module.c                          |  3 ++-
 kernel/module_signing.c                  | 11 ++++++-----
 6 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 63634c95265d..3aa5269f6e0f 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -158,7 +158,7 @@ int elf64_verify_sig(const char *kernel, unsigned long length)
 	size_t kernel_len = length;
 
 	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
-					"kexec_file");
+					 VERIFYING_KEXEC_APPENDED_SIGNATURE);
 }
 #endif /* CONFIG_KEXEC_SIG */
 
diff --git a/arch/s390/kernel/machine_kexec_file.c b/arch/s390/kernel/machine_kexec_file.c
index c4632c1a1b59..316d082c9d99 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -33,7 +33,7 @@ int s390_verify_sig(const char *kernel, unsigned long length)
 		return 0;
 
 	return verify_appended_signature(kernel, &kernel_len, VERIFY_USE_PLATFORM_KEYRING,
-					"kexec_file");
+					VERIFYING_KEXEC_APPENDED_SIGNATURE);
 }
 #endif /* CONFIG_KEXEC_SIG */
 
diff --git a/crypto/asymmetric_keys/asymmetric_type.c b/crypto/asymmetric_keys/asymmetric_type.c
index ad8af3d70ac0..6fd20eec3882 100644
--- a/crypto/asymmetric_keys/asymmetric_type.c
+++ b/crypto/asymmetric_keys/asymmetric_type.c
@@ -25,6 +25,7 @@ const char *const key_being_used_for[NR__KEY_BEING_USED_FOR] = {
 	[VERIFYING_KEXEC_PE_SIGNATURE]		= "kexec PE sig",
 	[VERIFYING_KEY_SIGNATURE]		= "key sig",
 	[VERIFYING_KEY_SELF_SIGNATURE]		= "key self sig",
+	[VERIFYING_KEXEC_APPENDED_SIGNATURE]	= "kexec appended sig",
 	[VERIFYING_UNSPECIFIED_SIGNATURE]	= "unspec sig",
 };
 EXPORT_SYMBOL_GPL(key_being_used_for);
diff --git a/include/linux/verification.h b/include/linux/verification.h
index c1cf0582012a..23748feb9e03 100644
--- a/include/linux/verification.h
+++ b/include/linux/verification.h
@@ -26,6 +26,7 @@ enum key_being_used_for {
 	VERIFYING_KEXEC_PE_SIGNATURE,
 	VERIFYING_KEY_SIGNATURE,
 	VERIFYING_KEY_SELF_SIGNATURE,
+	VERIFYING_KEXEC_APPENDED_SIGNATURE,
 	VERIFYING_UNSPECIFIED_SIGNATURE,
 	NR__KEY_BEING_USED_FOR
 };
@@ -61,7 +62,7 @@ extern int verify_pefile_signature(const void *pebuf, unsigned pelen,
 #endif
 
 int verify_appended_signature(const void *data, size_t *len, struct key *trusted_keys,
-			      const char *what);
+			      enum key_being_used_for purpose);
 
 #endif /* CONFIG_SYSTEM_DATA_VERIFICATION */
 #endif /* _LINUX_VERIFY_PEFILE_H */
diff --git a/kernel/module.c b/kernel/module.c
index d91ca0f93a40..0a359dc6b690 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2891,7 +2891,8 @@ static int module_sig_check(struct load_info *info, int flags)
 	 */
 	if (flags == 0) {
 		err = verify_appended_signature(mod, &info->len,
-						VERIFY_USE_SECONDARY_KEYRING, "module");
+						VERIFY_USE_SECONDARY_KEYRING,
+						VERIFYING_MODULE_SIGNATURE);
 		if (!err) {
 			info->sig_ok = true;
 			return 0;
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index 4c28cb55275f..cef72a6f6b5d 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -19,17 +19,18 @@
  * @data: The data to be verified
  * @len: Size of @data.
  * @trusted_keys: Keyring to use for verification
- * @what: Informational string for log messages
+ * @purpose: The use to which the key is being put
  */
 int verify_appended_signature(const void *data, size_t *len,
-			      struct key *trusted_keys, const char *what)
+			      struct key *trusted_keys,
+			      enum key_being_used_for purpose)
 {
 	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
 	struct module_signature ms;
 	size_t sig_len, modlen = *len;
 	int ret;
 
-	pr_devel("==>%s(,%zu)\n", __func__, modlen);
+	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);
 
 	if (markerlen > modlen)
 		return -ENODATA;
@@ -44,7 +45,7 @@ int verify_appended_signature(const void *data, size_t *len,
 
 	memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
 
-	ret = mod_check_sig(&ms, modlen, what);
+	ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
 	if (ret)
 		return ret;
 
@@ -54,6 +55,6 @@ int verify_appended_signature(const void *data, size_t *len,
 
 	return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
 				      trusted_keys,
-				      VERIFYING_MODULE_SIGNATURE,
+				      purpose,
 				      NULL, NULL);
 }
-- 
2.31.1

[PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig

[Michal Suchanek]

Multiple users of mod_check_sig check for the marker, then call
mod_check_sig, extract signature length, and remove the signature.

Put this code in one place together with mod_check_sig.

Signed-off-by: Michal Suchanek <msuchanek@suse.de>
---
 include/linux/module_signature.h    |  1 +
 kernel/module_signature.c           | 56 ++++++++++++++++++++++++++++-
 kernel/module_signing.c             | 26 +++-----------
 security/integrity/ima/ima_modsig.c | 22 ++----------
 4 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
index 7eb4b00381ac..1343879b72b3 100644
--- a/include/linux/module_signature.h
+++ b/include/linux/module_signature.h
@@ -42,5 +42,6 @@ struct module_signature {
 
 int mod_check_sig(const struct module_signature *ms, size_t file_len,
 		  const char *name);
+int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name);
 
 #endif /* _LINUX_MODULE_SIGNATURE_H */
diff --git a/kernel/module_signature.c b/kernel/module_signature.c
index 00132d12487c..784b40575ee4 100644
--- a/kernel/module_signature.c
+++ b/kernel/module_signature.c
@@ -8,14 +8,36 @@
 
 #include <linux/errno.h>
 #include <linux/printk.h>
+#include <linux/string.h>
 #include <linux/module_signature.h>
 #include <asm/byteorder.h>
 
+/**
+ * mod_check_sig_marker - check that the given data has signature marker at the end
+ *
+ * @data:	Data with appended signature
+ * @len:	Length of data. Signature marker length is subtracted on success.
+ */
+static inline int mod_check_sig_marker(const void *data, size_t *len)
+{
+	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+
+	if (markerlen > *len)
+		return -ENODATA;
+
+	if (memcmp(data + *len - markerlen, MODULE_SIG_STRING,
+		   markerlen))
+		return -ENODATA;
+
+	*len -= markerlen;
+	return 0;
+}
+
 /**
  * mod_check_sig - check that the given signature is sane
  *
  * @ms:		Signature to check.
- * @file_len:	Size of the file to which @ms is appended.
+ * @file_len:	Size of the file to which @ms is appended (without the marker).
  * @name:	What is being checked. Used for error messages.
  */
 int mod_check_sig(const struct module_signature *ms, size_t file_len,
@@ -44,3 +66,35 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
 
 	return 0;
 }
+
+/**
+ * mod_parse_sig - check that the given signature is sane and determine signature length
+ *
+ * @data:	Data with appended signature.
+ * @len:	Length of data. Signature and marker length is subtracted on success.
+ * @sig_len:	Length of signature. Filled on success.
+ * @name:	What is being checked. Used for error messages.
+ */
+int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name)
+{
+	const struct module_signature *sig;
+	int rc;
+
+	rc = mod_check_sig_marker(data, len);
+	if (rc)
+		return rc;
+
+	if (*len < sizeof(*sig))
+		return -ENODATA;
+
+	sig = (const struct module_signature *)(data + (*len - sizeof(*sig)));
+
+	rc = mod_check_sig(sig, *len, name);
+	if (rc)
+		return rc;
+
+	*sig_len = be32_to_cpu(sig->sig_len);
+	*len -= *sig_len + sizeof(*sig);
+
+	return 0;
+}
diff --git a/kernel/module_signing.c b/kernel/module_signing.c
index cef72a6f6b5d..02bbca90f467 100644
--- a/kernel/module_signing.c
+++ b/kernel/module_signing.c
@@ -25,35 +25,17 @@ int verify_appended_signature(const void *data, size_t *len,
 			      struct key *trusted_keys,
 			      enum key_being_used_for purpose)
 {
-	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
 	struct module_signature ms;
-	size_t sig_len, modlen = *len;
+	size_t sig_len;
 	int ret;
 
-	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);
+	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], *len);
 
-	if (markerlen > modlen)
-		return -ENODATA;
-
-	if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
-		   markerlen))
-		return -ENODATA;
-	modlen -= markerlen;
-
-	if (modlen <= sizeof(ms))
-		return -EBADMSG;
-
-	memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
-
-	ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
+	ret = mod_parse_sig(data, len, &sig_len, key_being_used_for[purpose]);
 	if (ret)
 		return ret;
 
-	sig_len = be32_to_cpu(ms.sig_len);
-	modlen -= sig_len + sizeof(ms);
-	*len = modlen;
-
-	return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
+	return verify_pkcs7_signature(data, *len, data + *len, sig_len,
 				      trusted_keys,
 				      purpose,
 				      NULL, NULL);
diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
index fb25723c65bc..46917eb37fd8 100644
--- a/security/integrity/ima/ima_modsig.c
+++ b/security/integrity/ima/ima_modsig.c
@@ -37,33 +37,17 @@ struct modsig {
  *
  * Return: 0 on success, error code otherwise.
  */
-int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
+int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t len,
 		    struct modsig **modsig)
 {
-	const size_t marker_len = strlen(MODULE_SIG_STRING);
-	const struct module_signature *sig;
 	struct modsig *hdr;
-	size_t sig_len;
-	const void *p;
+	size_t sig_len, buf_len = len;
 	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_parse_sig(buf, &buf_len, &sig_len, 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(sizeof(*hdr) + sig_len, GFP_KERNEL);
 	if (!hdr)
-- 
2.31.1

Re: [PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig

[Philipp Rudo]

Hi Michal,

On Thu, 25 Nov 2021 19:02:44 +0100
Michal Suchanek <msuchanek@suse.de> wrote:

> Multiple users of mod_check_sig check for the marker, then call
> mod_check_sig, extract signature length, and remove the signature.
> 
> Put this code in one place together with mod_check_sig.
> 
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
>  include/linux/module_signature.h    |  1 +
>  kernel/module_signature.c           | 56 ++++++++++++++++++++++++++++-
>  kernel/module_signing.c             | 26 +++-----------
>  security/integrity/ima/ima_modsig.c | 22 ++----------
>  4 files changed, 63 insertions(+), 42 deletions(-)
> 
> diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> index 7eb4b00381ac..1343879b72b3 100644
> --- a/include/linux/module_signature.h
> +++ b/include/linux/module_signature.h
> @@ -42,5 +42,6 @@ struct module_signature {
>  
>  int mod_check_sig(const struct module_signature *ms, size_t file_len,
>  		  const char *name);
> +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name);
>  
>  #endif /* _LINUX_MODULE_SIGNATURE_H */
> diff --git a/kernel/module_signature.c b/kernel/module_signature.c
> index 00132d12487c..784b40575ee4 100644
> --- a/kernel/module_signature.c
> +++ b/kernel/module_signature.c
> @@ -8,14 +8,36 @@
>  
>  #include <linux/errno.h>
>  #include <linux/printk.h>
> +#include <linux/string.h>
>  #include <linux/module_signature.h>
>  #include <asm/byteorder.h>
>  
> +/**
> + * mod_check_sig_marker - check that the given data has signature marker at the end
> + *
> + * @data:	Data with appended signature
> + * @len:	Length of data. Signature marker length is subtracted on success.
> + */
> +static inline int mod_check_sig_marker(const void *data, size_t *len)

I personally don't like it when a function has a "check" in it's name
as it doesn't describe what the function is checking for. For me
mod_has_sig_marker is much more precise. I would use that instead.

Thanks
Philipp

> +{
> +	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> +
> +	if (markerlen > *len)
> +		return -ENODATA;
> +
> +	if (memcmp(data + *len - markerlen, MODULE_SIG_STRING,
> +		   markerlen))
> +		return -ENODATA;
> +
> +	*len -= markerlen;
> +	return 0;
> +}
> +
>  /**
>   * mod_check_sig - check that the given signature is sane
>   *
>   * @ms:		Signature to check.
> - * @file_len:	Size of the file to which @ms is appended.
> + * @file_len:	Size of the file to which @ms is appended (without the marker).
>   * @name:	What is being checked. Used for error messages.
>   */
>  int mod_check_sig(const struct module_signature *ms, size_t file_len,
> @@ -44,3 +66,35 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
>  
>  	return 0;
>  }
> +
> +/**
> + * mod_parse_sig - check that the given signature is sane and determine signature length
> + *
> + * @data:	Data with appended signature.
> + * @len:	Length of data. Signature and marker length is subtracted on success.
> + * @sig_len:	Length of signature. Filled on success.
> + * @name:	What is being checked. Used for error messages.
> + */
> +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name)
> +{
> +	const struct module_signature *sig;
> +	int rc;
> +
> +	rc = mod_check_sig_marker(data, len);
> +	if (rc)
> +		return rc;
> +
> +	if (*len < sizeof(*sig))
> +		return -ENODATA;
> +
> +	sig = (const struct module_signature *)(data + (*len - sizeof(*sig)));
> +
> +	rc = mod_check_sig(sig, *len, name);
> +	if (rc)
> +		return rc;
> +
> +	*sig_len = be32_to_cpu(sig->sig_len);
> +	*len -= *sig_len + sizeof(*sig);
> +
> +	return 0;
> +}
> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index cef72a6f6b5d..02bbca90f467 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -25,35 +25,17 @@ int verify_appended_signature(const void *data, size_t *len,
>  			      struct key *trusted_keys,
>  			      enum key_being_used_for purpose)
>  {
> -	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
>  	struct module_signature ms;
> -	size_t sig_len, modlen = *len;
> +	size_t sig_len;
>  	int ret;
>  
> -	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);  
> +	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], *len);
>  
> -	if (markerlen > modlen)
> -		return -ENODATA;
> -
> -	if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> -		   markerlen))
> -		return -ENODATA;
> -	modlen -= markerlen;
> -
> -	if (modlen <= sizeof(ms))
> -		return -EBADMSG;
> -
> -	memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
> -
> -	ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
> +	ret = mod_parse_sig(data, len, &sig_len, key_being_used_for[purpose]);
>  	if (ret)
>  		return ret;
>  
> -	sig_len = be32_to_cpu(ms.sig_len);
> -	modlen -= sig_len + sizeof(ms);
> -	*len = modlen;
> -
> -	return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
> +	return verify_pkcs7_signature(data, *len, data + *len, sig_len,
>  				      trusted_keys,
>  				      purpose,
>  				      NULL, NULL);
> diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> index fb25723c65bc..46917eb37fd8 100644
> --- a/security/integrity/ima/ima_modsig.c
> +++ b/security/integrity/ima/ima_modsig.c
> @@ -37,33 +37,17 @@ struct modsig {
>   *
>   * Return: 0 on success, error code otherwise.
>   */
> -int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
> +int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t len,
>  		    struct modsig **modsig)
>  {
> -	const size_t marker_len = strlen(MODULE_SIG_STRING);
> -	const struct module_signature *sig;
>  	struct modsig *hdr;
> -	size_t sig_len;
> -	const void *p;
> +	size_t sig_len, buf_len = len;
>  	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_parse_sig(buf, &buf_len, &sig_len, 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(sizeof(*hdr) + sig_len, GFP_KERNEL);
>  	if (!hdr)

Re: [PATCH v2 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig

[Michal Suchánek]

Hello,

On Tue, Dec 07, 2021 at 05:10:34PM +0100, Philipp Rudo wrote:
> Hi Michal,
> 
> On Thu, 25 Nov 2021 19:02:44 +0100
> Michal Suchanek <msuchanek@suse.de> wrote:
> 
> > Multiple users of mod_check_sig check for the marker, then call
> > mod_check_sig, extract signature length, and remove the signature.
> > 
> > Put this code in one place together with mod_check_sig.
> > 
> > Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> > ---
> >  include/linux/module_signature.h    |  1 +
> >  kernel/module_signature.c           | 56 ++++++++++++++++++++++++++++-
> >  kernel/module_signing.c             | 26 +++-----------
> >  security/integrity/ima/ima_modsig.c | 22 ++----------
> >  4 files changed, 63 insertions(+), 42 deletions(-)
> > 
> > diff --git a/include/linux/module_signature.h b/include/linux/module_signature.h
> > index 7eb4b00381ac..1343879b72b3 100644
> > --- a/include/linux/module_signature.h
> > +++ b/include/linux/module_signature.h
> > @@ -42,5 +42,6 @@ struct module_signature {
> >  
> >  int mod_check_sig(const struct module_signature *ms, size_t file_len,
> >  		  const char *name);
> > +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name);
> >  
> >  #endif /* _LINUX_MODULE_SIGNATURE_H */
> > diff --git a/kernel/module_signature.c b/kernel/module_signature.c
> > index 00132d12487c..784b40575ee4 100644
> > --- a/kernel/module_signature.c
> > +++ b/kernel/module_signature.c
> > @@ -8,14 +8,36 @@
> >  
> >  #include <linux/errno.h>
> >  #include <linux/printk.h>
> > +#include <linux/string.h>
> >  #include <linux/module_signature.h>
> >  #include <asm/byteorder.h>
> >  
> > +/**
> > + * mod_check_sig_marker - check that the given data has signature marker at the end
> > + *
> > + * @data:	Data with appended signature
> > + * @len:	Length of data. Signature marker length is subtracted on success.
> > + */
> > +static inline int mod_check_sig_marker(const void *data, size_t *len)
> 
> I personally don't like it when a function has a "check" in it's name
> as it doesn't describe what the function is checking for. For me

It is consistent with mod_check_sig

> mod_has_sig_marker is much more precise. I would use that instead.

It actually would not because it does more than that.

Thanks

Michal

> 
> Thanks
> Philipp
> 
> > +{
> > +	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > +
> > +	if (markerlen > *len)
> > +		return -ENODATA;
> > +
> > +	if (memcmp(data + *len - markerlen, MODULE_SIG_STRING,
> > +		   markerlen))
> > +		return -ENODATA;
> > +
> > +	*len -= markerlen;
> > +	return 0;
> > +}
> > +
> >  /**
> >   * mod_check_sig - check that the given signature is sane
> >   *
> >   * @ms:		Signature to check.
> > - * @file_len:	Size of the file to which @ms is appended.
> > + * @file_len:	Size of the file to which @ms is appended (without the marker).
> >   * @name:	What is being checked. Used for error messages.
> >   */
> >  int mod_check_sig(const struct module_signature *ms, size_t file_len,
> > @@ -44,3 +66,35 @@ int mod_check_sig(const struct module_signature *ms, size_t file_len,
> >  
> >  	return 0;
> >  }
> > +
> > +/**
> > + * mod_parse_sig - check that the given signature is sane and determine signature length
> > + *
> > + * @data:	Data with appended signature.
> > + * @len:	Length of data. Signature and marker length is subtracted on success.
> > + * @sig_len:	Length of signature. Filled on success.
> > + * @name:	What is being checked. Used for error messages.
> > + */
> > +int mod_parse_sig(const void *data, size_t *len, size_t *sig_len, const char *name)
> > +{
> > +	const struct module_signature *sig;
> > +	int rc;
> > +
> > +	rc = mod_check_sig_marker(data, len);
> > +	if (rc)
> > +		return rc;
> > +
> > +	if (*len < sizeof(*sig))
> > +		return -ENODATA;
> > +
> > +	sig = (const struct module_signature *)(data + (*len - sizeof(*sig)));
> > +
> > +	rc = mod_check_sig(sig, *len, name);
> > +	if (rc)
> > +		return rc;
> > +
> > +	*sig_len = be32_to_cpu(sig->sig_len);
> > +	*len -= *sig_len + sizeof(*sig);
> > +
> > +	return 0;
> > +}
> > diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> > index cef72a6f6b5d..02bbca90f467 100644
> > --- a/kernel/module_signing.c
> > +++ b/kernel/module_signing.c
> > @@ -25,35 +25,17 @@ int verify_appended_signature(const void *data, size_t *len,
> >  			      struct key *trusted_keys,
> >  			      enum key_being_used_for purpose)
> >  {
> > -	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> >  	struct module_signature ms;
> > -	size_t sig_len, modlen = *len;
> > +	size_t sig_len;
> >  	int ret;
> >  
> > -	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], modlen);  
> > +	pr_devel("==>%s %s(,%zu)\n", __func__, key_being_used_for[purpose], *len);
> >  
> > -	if (markerlen > modlen)
> > -		return -ENODATA;
> > -
> > -	if (memcmp(data + modlen - markerlen, MODULE_SIG_STRING,
> > -		   markerlen))
> > -		return -ENODATA;
> > -	modlen -= markerlen;
> > -
> > -	if (modlen <= sizeof(ms))
> > -		return -EBADMSG;
> > -
> > -	memcpy(&ms, data + (modlen - sizeof(ms)), sizeof(ms));
> > -
> > -	ret = mod_check_sig(&ms, modlen, key_being_used_for[purpose]);
> > +	ret = mod_parse_sig(data, len, &sig_len, key_being_used_for[purpose]);
> >  	if (ret)
> >  		return ret;
> >  
> > -	sig_len = be32_to_cpu(ms.sig_len);
> > -	modlen -= sig_len + sizeof(ms);
> > -	*len = modlen;
> > -
> > -	return verify_pkcs7_signature(data, modlen, data + modlen, sig_len,
> > +	return verify_pkcs7_signature(data, *len, data + *len, sig_len,
> >  				      trusted_keys,
> >  				      purpose,
> >  				      NULL, NULL);
> > diff --git a/security/integrity/ima/ima_modsig.c b/security/integrity/ima/ima_modsig.c
> > index fb25723c65bc..46917eb37fd8 100644
> > --- a/security/integrity/ima/ima_modsig.c
> > +++ b/security/integrity/ima/ima_modsig.c
> > @@ -37,33 +37,17 @@ struct modsig {
> >   *
> >   * Return: 0 on success, error code otherwise.
> >   */
> > -int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t buf_len,
> > +int ima_read_modsig(enum ima_hooks func, const void *buf, loff_t len,
> >  		    struct modsig **modsig)
> >  {
> > -	const size_t marker_len = strlen(MODULE_SIG_STRING);
> > -	const struct module_signature *sig;
> >  	struct modsig *hdr;
> > -	size_t sig_len;
> > -	const void *p;
> > +	size_t sig_len, buf_len = len;
> >  	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_parse_sig(buf, &buf_len, &sig_len, 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(sizeof(*hdr) + sig_len, GFP_KERNEL);
> >  	if (!hdr)
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Heiko Carstens]

On Thu, Nov 25, 2021 at 07:02:38PM +0100, Michal Suchanek wrote:
> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)

For all patches which touch s390:
Acked-by: Heiko Carstens <hca@linux.ibm.com>

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Baoquan He]

Hi,

On 11/25/21 at 07:02pm, Michal Suchanek wrote:
> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.

Do you have the link of your 1st version?

And after going through the whole series, it doesn't tell what this
patch series intends to do in cover-letter or patch log.

Thanks
Baoquan

> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)
> 
> -- 
> 2.31.1
> 
> 
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Michal Suchánek]

Hello,

On Wed, Dec 01, 2021 at 10:37:47AM +0800, Baoquan He wrote:
> Hi,
> 
> On 11/25/21 at 07:02pm, Michal Suchanek wrote:
> > Hello,
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> 
> Do you have the link of your 1st version?

This is the previous version:
https://lore.kernel.org/lkml/cover.1635948742.git.msuchanek@suse.de/

Thanks

Michal

> And after going through the whole series, it doesn't tell what this
> patch series intends to do in cover-letter or patch log.
> 
> Thanks
> Baoquan
> 
> > 
> > The first two patches can be applied separately without the rest.
> > 
> > Thanks
> > 
> > Michal
> > 
> > Michal Suchanek (6):
> >   s390/kexec_file: Don't opencode appended signature check.
> >   powerpc/kexec_file: Add KEXEC_SIG support.
> >   kexec_file: Don't opencode appended signature verification.
> >   module: strip the signature marker in the verification function.
> >   module: Use key_being_used_for for log messages in
> >     verify_appended_signature
> >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > 
> >  arch/powerpc/Kconfig                     | 11 +++++
> >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> >  include/linux/module_signature.h         |  1 +
> >  include/linux/verification.h             |  4 ++
> >  kernel/module-internal.h                 |  2 -
> >  kernel/module.c                          | 12 +++--
> >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> >  kernel/module_signing.c                  | 33 +++++++-------
> >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> >  11 files changed, 113 insertions(+), 85 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 
> > 
> > _______________________________________________
> > kexec mailing list
> > kexec@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/kexec
> > 
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Philipp Rudo]

Hi Michal,

i finally had the time to take a closer look at the series. Except for
the nit in patch 4 and my personal preference in patch 6 the code looks
good to me.

What I don't like are the commit messages on the first commits. In my
opinion they are so short that they are almost useless. For example in
patch 2 there is absolutely no explanation why you can simply copy the
s390 over to ppc. Or in patch 3 you are silently changing the error
code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if
you could improve them a little.

Thanks
Philipp

On Thu, 25 Nov 2021 19:02:38 +0100
Michal Suchanek <msuchanek@suse.de> wrote:

> Hello,
> 
> This is resend of the KEXEC_SIG patchset.
> 
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
> 
> The second patch is the only one that is intended to change any
> functionality.
> 
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
> 
> The first two patches can be applied separately without the rest.
> 
> Thanks
> 
> Michal
> 
> Michal Suchanek (6):
>   s390/kexec_file: Don't opencode appended signature check.
>   powerpc/kexec_file: Add KEXEC_SIG support.
>   kexec_file: Don't opencode appended signature verification.
>   module: strip the signature marker in the verification function.
>   module: Use key_being_used_for for log messages in
>     verify_appended_signature
>   module: Move duplicate mod_check_sig users code to mod_parse_sig
> 
>  arch/powerpc/Kconfig                     | 11 +++++
>  arch/powerpc/kexec/elf_64.c              | 14 ++++++
>  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
>  crypto/asymmetric_keys/asymmetric_type.c |  1 +
>  include/linux/module_signature.h         |  1 +
>  include/linux/verification.h             |  4 ++
>  kernel/module-internal.h                 |  2 -
>  kernel/module.c                          | 12 +++--
>  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
>  kernel/module_signing.c                  | 33 +++++++-------
>  security/integrity/ima/ima_modsig.c      | 22 ++--------
>  11 files changed, 113 insertions(+), 85 deletions(-)
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Michal Suchánek]

On Tue, Dec 07, 2021 at 05:10:14PM +0100, Philipp Rudo wrote:
> Hi Michal,
> 
> i finally had the time to take a closer look at the series. Except for
> the nit in patch 4 and my personal preference in patch 6 the code looks
> good to me.
> 
> What I don't like are the commit messages on the first commits. In my
> opinion they are so short that they are almost useless. For example in
> patch 2 there is absolutely no explanation why you can simply copy the
> s390 over to ppc.

They use the same signature format. I suppose I can add a note saying
that.

> Or in patch 3 you are silently changing the error
> code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if

Not sure what I should do about this. The different implementations use
different random error codes, and when they are unified the error code
clearly changes for one or the other.

Does anything depend on a particular error code returned?

Thanks

Michal

> you could improve them a little.
> 
> Thanks
> Philipp
> 
> On Thu, 25 Nov 2021 19:02:38 +0100
> Michal Suchanek <msuchanek@suse.de> wrote:
> 
> > Hello,
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> > 
> > The first two patches can be applied separately without the rest.
> > 
> > Thanks
> > 
> > Michal
> > 
> > Michal Suchanek (6):
> >   s390/kexec_file: Don't opencode appended signature check.
> >   powerpc/kexec_file: Add KEXEC_SIG support.
> >   kexec_file: Don't opencode appended signature verification.
> >   module: strip the signature marker in the verification function.
> >   module: Use key_being_used_for for log messages in
> >     verify_appended_signature
> >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > 
> >  arch/powerpc/Kconfig                     | 11 +++++
> >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> >  include/linux/module_signature.h         |  1 +
> >  include/linux/verification.h             |  4 ++
> >  kernel/module-internal.h                 |  2 -
> >  kernel/module.c                          | 12 +++--
> >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> >  kernel/module_signing.c                  | 33 +++++++-------
> >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> >  11 files changed, 113 insertions(+), 85 deletions(-)
> > 
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Philipp Rudo]

Hi Michal,

On Tue, 7 Dec 2021 18:32:21 +0100
Michal Suchánek <msuchanek@suse.de> wrote:

> On Tue, Dec 07, 2021 at 05:10:14PM +0100, Philipp Rudo wrote:
> > Hi Michal,
> > 
> > i finally had the time to take a closer look at the series. Except for
> > the nit in patch 4 and my personal preference in patch 6 the code looks
> > good to me.
> > 
> > What I don't like are the commit messages on the first commits. In my
> > opinion they are so short that they are almost useless. For example in
> > patch 2 there is absolutely no explanation why you can simply copy the
> > s390 over to ppc.  
> 
> They use the same signature format. I suppose I can add a note saying
> that.

The note is what I was asking for. For me the commit message is an
important piece of documentation for other developers (or yourself in a
year). That's why in my opinion it's important to describe _why_ you do
something in it as you cannot get the _why_ by reading the code.

> > Or in patch 3 you are silently changing the error
> > code in kexec from EKEYREJECT to ENODATA. So I would appreciate it if  
> 
> Not sure what I should do about this. The different implementations use
> different random error codes, and when they are unified the error code
> clearly changes for one or the other.

My complaint wasn't that you change the return code. There's no way to
avoid choosing one over the other. It's again that you don't document
the change in the commit message for others.

> Does anything depend on a particular error code returned?

Not that I know of. At least in the kexec-tools ENODATA and EKEYREJECT
are handled the same way.

Thanks
Philipp


> Thanks
> 
> Michal
> 
> > you could improve them a little.
> > 
> > Thanks
> > Philipp
> > 
> > On Thu, 25 Nov 2021 19:02:38 +0100
> > Michal Suchanek <msuchanek@suse.de> wrote:
> >   
> > > Hello,
> > > 
> > > This is resend of the KEXEC_SIG patchset.
> > > 
> > > The first patch is new because it'a a cleanup that does not require any
> > > change to the module verification code.
> > > 
> > > The second patch is the only one that is intended to change any
> > > functionality.
> > > 
> > > The rest only deduplicates code but I did not receive any review on that
> > > part so I don't know if it's desirable as implemented.
> > > 
> > > The first two patches can be applied separately without the rest.
> > > 
> > > Thanks
> > > 
> > > Michal
> > > 
> > > Michal Suchanek (6):
> > >   s390/kexec_file: Don't opencode appended signature check.
> > >   powerpc/kexec_file: Add KEXEC_SIG support.
> > >   kexec_file: Don't opencode appended signature verification.
> > >   module: strip the signature marker in the verification function.
> > >   module: Use key_being_used_for for log messages in
> > >     verify_appended_signature
> > >   module: Move duplicate mod_check_sig users code to mod_parse_sig
> > > 
> > >  arch/powerpc/Kconfig                     | 11 +++++
> > >  arch/powerpc/kexec/elf_64.c              | 14 ++++++
> > >  arch/s390/kernel/machine_kexec_file.c    | 42 ++----------------
> > >  crypto/asymmetric_keys/asymmetric_type.c |  1 +
> > >  include/linux/module_signature.h         |  1 +
> > >  include/linux/verification.h             |  4 ++
> > >  kernel/module-internal.h                 |  2 -
> > >  kernel/module.c                          | 12 +++--
> > >  kernel/module_signature.c                | 56 +++++++++++++++++++++++-
> > >  kernel/module_signing.c                  | 33 +++++++-------
> > >  security/integrity/ima/ima_modsig.c      | 22 ++--------
> > >  11 files changed, 113 insertions(+), 85 deletions(-)
> > >   
> >   
> 

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Nayna]

On 11/25/21 13:02, Michal Suchanek wrote:
> Hello,

Hi Michael,

>
> This is resend of the KEXEC_SIG patchset.
>
> The first patch is new because it'a a cleanup that does not require any
> change to the module verification code.
>
> The second patch is the only one that is intended to change any
> functionality.
>
> The rest only deduplicates code but I did not receive any review on that
> part so I don't know if it's desirable as implemented.
>
> The first two patches can be applied separately without the rest.

Patch 2 fails to apply on v5.16-rc4. Can you please also include git 
tree/branch while posting the patches ?

Secondly, I see that you add the powerpc support in Patch 2 and then 
modify it again in Patch 5 after cleanup. Why not add the support for 
powerpc after the clean up ? This will reduce some rework and also 
probably simplify patches.

Thanks & Regards,

      - Nayna

Re: [PATCH v2 0/6] KEXEC_SIG with appended signature

[Michal Suchánek]

Hello,

On Wed, Dec 08, 2021 at 08:50:54PM -0500, Nayna wrote:
> 
> On 11/25/21 13:02, Michal Suchanek wrote:
> > Hello,
> 
> Hi Michael,
> 
> > 
> > This is resend of the KEXEC_SIG patchset.
> > 
> > The first patch is new because it'a a cleanup that does not require any
> > change to the module verification code.
> > 
> > The second patch is the only one that is intended to change any
> > functionality.
> > 
> > The rest only deduplicates code but I did not receive any review on that
> > part so I don't know if it's desirable as implemented.
> > 
> > The first two patches can be applied separately without the rest.
> 
> Patch 2 fails to apply on v5.16-rc4. Can you please also include git
> tree/branch while posting the patches ?

Sorry, I did not have a clean base and the Kconfig had another change.

Here is a tree with the changes applied:
https://github.com/hramrach/kernel/tree/kexec_sig

> 
> Secondly, I see that you add the powerpc support in Patch 2 and then modify
> it again in Patch 5 after cleanup. Why not add the support for powerpc after
> the clean up ? This will reduce some rework and also probably simplify
> patches.

That's because I don't know if the later patches will be accepted. By
queueing this patch first it can be applied standalone to ppc tree
without regard for the other patches. It's a copy of the s390 code so it
needs the same rework - not really adding complexity.

Thanks

Michal