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

2022-02-03 Thread Michal Suchánek
Hello,

thanks for the review.

On Tue, Jan 25, 2022 at 12:15:56PM -0800, Luis Chamberlain wrote:
> On Tue, Jan 11, 2022 at 12:37:45PM +0100, Michal Suchanek wrote:
> > diff --git a/include/linux/verification.h b/include/linux/verification.h
> > index a655923335ae..32db9287a7b0 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, unsigned long *len,
> > + struct key *trusted_keys, const char *what);
> > +
> 
> Looks very non-module specific.

Which it is now that the same signature format is used for kernels.

> 
> > diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> > index 8723ae70ea1f..30149969f21f 100644
> > --- a/kernel/module_signing.c
> > +++ b/kernel/module_signing.c
> > @@ -14,32 +14,38 @@
> >  #include 
> >  #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, unsigned long *len,
> > + struct key *trusted_keys, const char *what)
> >  {
> > -   struct module_signature ms;
> > -   size_t sig_len, modlen = info->len;
> > +   struct module_signature *ms;
> 
> There goes the abstraction, so why not make this clear where we re-use
> the struct module_signature for various things and call it as it is,
> verify_mod_appended_signature() or some such?

It sounds like the abstraction is actually improved by callers no longer
dealing with struct module_signature when verifying signature on a
kernel. That is the structure is misnamed but it is now hidden behind
an abstraction.

Or am I missing something?

Thanks

Michal


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

2022-01-25 Thread Luis Chamberlain
On Tue, Jan 11, 2022 at 12:37:45PM +0100, Michal Suchanek wrote:
> diff --git a/include/linux/verification.h b/include/linux/verification.h
> index a655923335ae..32db9287a7b0 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, unsigned long *len,
> +   struct key *trusted_keys, const char *what);
> +

Looks very non-module specific.

> diff --git a/kernel/module_signing.c b/kernel/module_signing.c
> index 8723ae70ea1f..30149969f21f 100644
> --- a/kernel/module_signing.c
> +++ b/kernel/module_signing.c
> @@ -14,32 +14,38 @@
>  #include 
>  #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, unsigned long *len,
> +   struct key *trusted_keys, const char *what)
>  {
> - struct module_signature ms;
> - size_t sig_len, modlen = info->len;
> + struct module_signature *ms;

There goes the abstraction, so why not make this clear where we re-use
the struct module_signature for various things and call it as it is,
verify_mod_appended_signature() or some such?

David? Any preference?

Other than that:

Reviewed-by: Luis Chamberlain 

  Luis


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

2022-01-11 Thread Michal Suchanek
Module verification already implements appeded signature verification.

Reuse it for kexec_file.

Signed-off-by: Michal Suchanek 
---
v3: - Philipp Rudo : Update the dependency on
  MODULE_SIG_FORMAT to MODULE_SIG
- Include linux/verification.h - previously added in earlier patch
v4: - kernel test robot : Use unsigned long rather than size_t 
for data length
- Update the code in kernel/module_signing.c to use pointer rather
  than memcpy as the kexec and IMA code does
---
 arch/powerpc/Kconfig  |  2 +-
 arch/powerpc/kexec/elf_64.c   | 19 +++
 arch/s390/Kconfig |  2 +-
 arch/s390/kernel/machine_kexec_file.c | 18 ++
 include/linux/verification.h  |  3 +++
 kernel/module-internal.h  |  2 --
 kernel/module.c   |  4 +++-
 kernel/module_signing.c   | 34 ---
 8 files changed, 33 insertions(+), 51 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 1cde9b6c5987..4092187474ff 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -562,7 +562,7 @@ config ARCH_HAS_KEXEC_PURGATORY
 
 config KEXEC_SIG
bool "Verify kernel signature during kexec_file_load() syscall"
-   depends on KEXEC_FILE && MODULE_SIG_FORMAT
+   depends on KEXEC_FILE && MODULE_SIG
help
  This option makes kernel signature verification mandatory for
  the kexec_file_load() syscall.
diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c
index 98d1cb5135b4..64cd314cce0d 100644
--- a/arch/powerpc/kexec/elf_64.c
+++ b/arch/powerpc/kexec/elf_64.c
@@ -23,6 +23,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 static void *elf64_load(struct kimage *image, char *kernel_buf,
@@ -156,9 +157,6 @@ static void *elf64_load(struct kimage *image, char 
*kernel_buf,
 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;
@@ -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/Kconfig b/arch/s390/Kconfig
index 2a5bb4f29cfe..cece7152ea35 100644
--- a/arch/s390/Kconfig
+++ b/arch/s390/Kconfig
@@ -544,7 +544,7 @@ config ARCH_HAS_KEXEC_PURGATORY
 
 config KEXEC_SIG
bool "Verify kernel signature during kexec_file_load() syscall"
-   depends on KEXEC_FILE && MODULE_SIG_FORMAT
+   depends on KEXEC_FILE && MODULE_SIG
help
  This option makes kernel signature verification mandatory for
  the kexec_file_load() syscall.
diff --git a/arch/s390/kernel/machine_kexec_file.c 
b/arch/s390/kernel/machine_kexec_file.c
index c944d71316c7..345f2eab6e04 100644
--- a/arch/s390/kernel/machine_kexec_file.c
+++ b/arch/s390/kernel/machine_kexec_file.c
@@ -29,9 +29,6 @@ const struct kexec_file_ops * const kexec_file_loaders[] = {
 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)
@@ -45,19 +42,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,