Re: [PATCH v2 0/6] KEXEC_SIG with appended signature
Hi Michal, On Tue, 7 Dec 2021 18:32:21 +0100 Michal Suchánek 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 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 4/6] module: strip the signature marker in the verification function.
Hi Michal,
On Thu, 25 Nov 2021 19:02:42 +0100
Michal Suchanek wrote:
> It is stripped by each caller separately.
>
> Signed-off-by: Michal Suchanek
> ---
> 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;
> +
>
Re: [PATCH v2 0/6] KEXEC_SIG with appended signature
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 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 6/6] module: Move duplicate mod_check_sig users code to mod_parse_sig
Hi Michal,
On Thu, 25 Nov 2021 19:02:44 +0100
Michal Suchanek 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
> ---
> 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
> #include
> +#include
> #include
> #include
>
> +/**
> + * 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,
Re: [PATCH 0/3] KEXEC_SIG with appended signature
Hi Michal, On Wed, 24 Nov 2021 14:27:16 +0100 Michal Suchánek wrote: > On Wed, Nov 24, 2021 at 08:10:10AM -0500, Mimi Zohar wrote: > > On Wed, 2021-11-24 at 12:09 +0100, Philipp Rudo wrote: > > > Now Michal wants to adapt KEXEC_SIG for ppc too so distros can rely on all > > > architectures using the same mechanism and thus reduce maintenance cost. > > > On the way there he even makes some absolutely reasonable improvements > > > for everybody. > > > > > > Why is that so controversial? What is the real problem that should be > > > discussed here? > > > > Nothing is controversial with what Michal wants to do. I've already > > said, "As for adding KEXEC_SIG appended signature support on PowerPC > > based on the s390 code, it sounds reasonable." > > Ok, I will resend the series with the arch-specific changes first to be > independent of the core cleanup. could you please add the ke...@lists.infradead.org to Cc when you resend the series. As this is kexec related I think it makes sense to give them a heads up, too. Thanks Philipp
Re: [PATCH 0/3] KEXEC_SIG with appended signature
Hi Mimi,
On Fri, 19 Nov 2021 13:16:20 -0500
Mimi Zohar wrote:
> On Fri, 2021-11-19 at 12:18 +0100, Michal Suchánek wrote:
> > Maybe I was not clear enough. If you happen to focus on an architecture
> > that supports IMA fully it's great.
> >
> > My point of view is maintaining multiple architectures. Both end users
> > and people conecerend with security are rarely familiar with
> > architecture specifics. Portability of documentation and debugging
> > instructions across architectures is a concern.
> >
> > IMA has large number of options with varying availablitily across
> > architectures for no apparent reason. The situation is complex and hard
> > to grasp.
>
> IMA measures, verifies, and audits the integrity of files based on a
> system wide policy. The known "good" integrity value may be stored in
> the security.ima xattr or more recently as an appended signature.
>
> With both IMA kexec appraise and measurement policy rules, not only is
> the kernel image signature verified and the file hash included in the
> IMA measurement list, but the signature used to verify the integrity of
> the kexec kernel image is also included in the IMA measurement list
> (ima_template=ima-sig).
>
> Even without PECOFF support in IMA, IMA kexec measurement policy rules
> can be defined to supplement the KEXEC_SIG signature verfication.
>
> > In comparison the *_SIG options are widely available. The missing
> > support for KEXEC_SIG on POWER is trivial to add by cut&paste from s390.
> > With that all the documentation that exists already is also trivially
> > applicable to POWER. Any additional code cleanup is a bonus but not
> > really needed to enable the kexec lockdown on POWER.
>
> Before lockdown was upstreamed, Matthew made sure that IMA signature
> verification could co-exist. Refer to commit 29d3c1c8dfe7 ("kexec:
> Allow kexec_file() with appropriate IMA policy when locked down"). If
> there is a problem with the downstream kexec lockdown patches, they
> should be fixed.
>
> The kexec kselftest might provide some insight into how the different
> signature verification methods and lockdown co-exist.
>
> As for adding KEXEC_SIG appended signature support on PowerPC based on
> the s390 code, it sounds reasonable.
Heiko contacted me as you apparently requested that someone from s390
takes part in this discussion. I now spend over a day trying to make
any sens from this discussion but failed. The way I see it is.
On one hand there is KEXEC_SIG which is specifically designed to verify
the signatures of kernel images in kexec_file_load. From the beginning
it was designed in a way that every architecture (in fact even every
image type) can define its own callback function as needed. It's design
is simple and easy to extend and thus was adopted by all architectures,
except ppc, so far.
On the other hand there is IMA which is much more general and also
includes the same functionality like KEXEC_SIG but only on some
architectures in some special cases and without proper documentation.
Now Michal wants to adapt KEXEC_SIG for ppc too so distros can rely on all
architectures using the same mechanism and thus reduce maintenance cost.
On the way there he even makes some absolutely reasonable improvements
for everybody.
Why is that so controversial? What is the real problem that should be
discussed here?
Thanks
Philipp
Re: [PATCH v3] ima: add a new CONFIG for loading arch-specific policies
On Sun, 8 Mar 2020 20:57:51 -0400 Nayna Jain wrote: > From: Nayna Jain > > Every time a new architecture defines the IMA architecture specific > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > include file needs to be updated. To avoid this "noise", this patch > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > the different architectures to select it. > > Suggested-by: Linus Torvalds > Signed-off-by: Nayna Jain > Acked-by: Ard Biesheuvel > Cc: Ard Biesheuvel > Cc: Philipp Rudo > Cc: Michael Ellerman > --- > v3: > * Removes CONFIG_IMA dependency. Thanks Ard. > * Updated the patch with improvements suggested by Michael. It now uses > "imply" instead of "select". Thanks Michael. > * Replaced the CONFIG_IMA in x86 and s390 with new config, else it was > resulting in redefinition when the IMA_SECURE_AND_OR_TRUSTED_BOOT > is not enabled. Thanks to Mimi for identifying the problem. > * Removed "#ifdef EFI" check in the arch/x86/Makefile for compiling > ima_arch.c file. > * Ard, Thanks for your Acked-by. I have changed the arch/x86/Makefile in > this version. Can you please review again and confirm ? > * Rudo, Thanks for your review. I have changed arch/s390/Makefile as well. > Can you also please review again ? Looks good to me for the s390 part Acked-by: Philipp Rudo Thanks Philipp > > v2: > * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael for > discussing the fix. > > arch/powerpc/Kconfig | 1 + > arch/s390/Kconfig | 1 + > arch/s390/kernel/Makefile | 2 +- > arch/x86/Kconfig | 1 + > arch/x86/kernel/Makefile | 4 +--- > include/linux/ima.h| 3 +-- > security/integrity/ima/Kconfig | 7 +++ > 7 files changed, 13 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > index 497b7d0b2d7e..5b9f1cba2a44 100644 > --- a/arch/powerpc/Kconfig > +++ b/arch/powerpc/Kconfig > @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT > bool > depends on PPC_POWERNV > depends on IMA_ARCH_POLICY > + imply IMA_SECURE_AND_OR_TRUSTED_BOOT > help > Systems with firmware secure boot enabled need to define security > policies to extend secure boot to the OS. This config allows a user > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > index 8abe77536d9d..59c216af6264 100644 > --- a/arch/s390/Kconfig > +++ b/arch/s390/Kconfig > @@ -195,6 +195,7 @@ config S390 > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > select SWIOTLB > select GENERIC_ALLOCATOR > + imply IMA_SECURE_AND_OR_TRUSTED_BOOT > > > config SCHED_OMIT_FRAME_POINTER > diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile > index 2b1203cf7be6..578a6fa82ea4 100644 > --- a/arch/s390/kernel/Makefile > +++ b/arch/s390/kernel/Makefile > @@ -70,7 +70,7 @@ obj-$(CONFIG_JUMP_LABEL)+= jump_label.o > obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o > obj-$(CONFIG_KEXEC_FILE) += kexec_elf.o > > -obj-$(CONFIG_IMA)+= ima_arch.o > +obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o > > obj-$(CONFIG_PERF_EVENTS)+= perf_event.o perf_cpum_cf_common.o > obj-$(CONFIG_PERF_EVENTS)+= perf_cpum_cf.o perf_cpum_sf.o > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > index beea77046f9b..dcf5b1729f7c 100644 > --- a/arch/x86/Kconfig > +++ b/arch/x86/Kconfig > @@ -230,6 +230,7 @@ config X86 > select VIRT_TO_BUS > select X86_FEATURE_NAMESif PROC_FS > select PROC_PID_ARCH_STATUS if PROC_FS > + imply IMA_SECURE_AND_OR_TRUSTED_BOOTif EFI > > config INSTRUCTION_DECODER > def_bool y > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile > index 9b294c13809a..cfef37a27def 100644 > --- a/arch/x86/kernel/Makefile > +++ b/arch/x86/kernel/Makefile > @@ -154,6 +154,4 @@ ifeq ($(CONFIG_X86_64),y) > obj-y += vsmp_64.o > endif > > -ifdef CONFIG_EFI > -obj-$(CONFIG_IMA)+= ima_arch.o > -endif > +obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o > diff --git a/include/linux/ima.h b/include/linux/ima.h > index 1659217e9b60..aefe758f4466 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int size); > extern void ima_add_kexec_buffer(struct kimage *image); > #endif > > -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \ > - || defined(CONFIG_PPC_SECURE_BOOT) &
Re: [PATCH v2] ima: add a new CONFIG for loading arch-specific policies
On Wed, 04 Mar 2020 07:55:38 -0500 Mimi Zohar wrote: > [Cc'ing Thomas Gleixner and x86 mailing list] > > On Wed, 2020-03-04 at 08:14 +0100, Ard Biesheuvel wrote: > > On Wed, 4 Mar 2020 at 03:34, Nayna Jain wrote: > > > > > > Every time a new architecture defines the IMA architecture specific > > > functions - arch_ima_get_secureboot() and arch_ima_get_policy(), the IMA > > > include file needs to be updated. To avoid this "noise", this patch > > > defines a new IMA Kconfig IMA_SECURE_AND_OR_TRUSTED_BOOT option, allowing > > > the different architectures to select it. > > > > > > Suggested-by: Linus Torvalds > > > Signed-off-by: Nayna Jain > > > Cc: Ard Biesheuvel > > > Cc: Philipp Rudo > > > Cc: Michael Ellerman > > > > Acked-by: Ard Biesheuvel > > Thanks, Ard. > > > > for the x86 bits, but I'm not an x86 maintainer. Also, you may need to > > split this if you want to permit arch maintainers to pick up their > > parts individually. > > Michael, Philipp, Thomas, do you prefer separate patches? I don't think splitting this patch makes sense. Otherwise you would break the build for all architectures until they picked up their line of code. I'm fine with the patch as is. Thanks Philipp > > > > > --- > > > v2: > > > * Fixed the issue identified by Mimi. Thanks Mimi, Ard, Heiko and Michael > > > for > > > discussing the fix. > > > > > > arch/powerpc/Kconfig | 1 + > > > arch/s390/Kconfig | 1 + > > > arch/x86/Kconfig | 1 + > > > include/linux/ima.h| 3 +-- > > > security/integrity/ima/Kconfig | 9 + > > > 5 files changed, 13 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig > > > index 497b7d0b2d7e..a5cfde432983 100644 > > > --- a/arch/powerpc/Kconfig > > > +++ b/arch/powerpc/Kconfig > > > @@ -979,6 +979,7 @@ config PPC_SECURE_BOOT > > > bool > > > depends on PPC_POWERNV > > > depends on IMA_ARCH_POLICY > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT > > > help > > > Systems with firmware secure boot enabled need to define > > > security > > > policies to extend secure boot to the OS. This config allows a > > > user > > > diff --git a/arch/s390/Kconfig b/arch/s390/Kconfig > > > index 8abe77536d9d..4a502fbcb800 100644 > > > --- a/arch/s390/Kconfig > > > +++ b/arch/s390/Kconfig > > > @@ -195,6 +195,7 @@ config S390 > > > select ARCH_HAS_FORCE_DMA_UNENCRYPTED > > > select SWIOTLB > > > select GENERIC_ALLOCATOR > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if IMA_ARCH_POLICY > > > > > > > > > config SCHED_OMIT_FRAME_POINTER > > > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig > > > index beea77046f9b..7f5bfaf0cbd2 100644 > > > --- a/arch/x86/Kconfig > > > +++ b/arch/x86/Kconfig > > > @@ -230,6 +230,7 @@ config X86 > > > select VIRT_TO_BUS > > > select X86_FEATURE_NAMESif PROC_FS > > > select PROC_PID_ARCH_STATUS if PROC_FS > > > + select IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI && IMA_ARCH_POLICY > > > > > > config INSTRUCTION_DECODER > > > def_bool y > > > diff --git a/include/linux/ima.h b/include/linux/ima.h > > > index 1659217e9b60..aefe758f4466 100644 > > > --- a/include/linux/ima.h > > > +++ b/include/linux/ima.h > > > @@ -30,8 +30,7 @@ extern void ima_kexec_cmdline(const void *buf, int > > > size); > > > extern void ima_add_kexec_buffer(struct kimage *image); > > > #endif > > > > > > -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) > > > \ > > > - || defined(CONFIG_PPC_SECURE_BOOT) > > > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT > > > extern bool arch_ima_get_secureboot(void); > > > extern const char * const *arch_get_ima_policy(void); > > > #else > > > diff --git a/security/integrity/ima/Kconfig > > > b/security/integrity/ima/Kconfig > > > index 3f3ee4e2eb0d..d17972aa413a 100644 > > > --- a/security/integrity/ima/Kconfig > > > +++ b/security/integrity/ima/Kconfig > > > @@ -327,3 +327,12 @@ config IMA_QUEUE_EARLY_BOOT_KEYS > > > depends on IMA_MEASURE_ASYMMETRIC_KEYS > > > depends on SYSTEM_TRUSTED_KEYRING > > > default y > > > + > > > +config IMA_SECURE_AND_OR_TRUSTED_BOOT > > > + bool > > > + depends on IMA > > > + depends on IMA_ARCH_POLICY > > > > Doesn't the latter already depend on the former? > > Yes, there's no need for the first. > > Mimi > > > > > + default n > > > + help > > > + This option is selected by architectures to enable secure > > > and/or > > > + trusted boot based on IMA runtime policies. > > > -- > > > 2.13.6 > > > >
Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
Hi Thiago, > > The patch looks good now. > > Thanks! Can I add your Reviewed-by? sorry, for the late answer, but I was on vacation the last two weeks. I hope it's not too late now. Reviewed-by: Philipp Rudo
Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
Hi Thiago, On Thu, 04 Jul 2019 15:57:34 -0300 Thiago Jung Bauermann wrote: > Hello Philipp, > > Philipp Rudo writes: > > > Hi Thiago, > > > > > > On Thu, 04 Jul 2019 03:42:57 -0300 > > Thiago Jung Bauermann wrote: > > > >> Jessica Yu writes: > >> > >> > +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]: > >> >>IMA will use the module_signature format for append signatures, so export > >> >>the relevant definitions and factor out the code which verifies that the > >> >>appended signature trailer is valid. > >> >> > >> >>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it > >> >>and be able to use mod_check_sig() without having to depend on either > >> >>CONFIG_MODULE_SIG or CONFIG_MODULES. > >> >> > >> >>Signed-off-by: Thiago Jung Bauermann > >> >>Reviewed-by: Mimi Zohar > >> >>Cc: Jessica Yu > >> >>--- > >> >> include/linux/module.h | 3 -- > >> >> include/linux/module_signature.h | 44 + > >> >> init/Kconfig | 6 +++- > >> >> kernel/Makefile | 1 + > >> >> kernel/module.c | 1 + > >> >> kernel/module_signature.c| 46 ++ > >> >> kernel/module_signing.c | 56 +--- > >> >> scripts/Makefile | 2 +- > >> >> 8 files changed, 106 insertions(+), 53 deletions(-) > >> >> > >> >>diff --git a/include/linux/module.h b/include/linux/module.h > >> >>index 188998d3dca9..aa56f531cf1e 100644 > >> >>--- a/include/linux/module.h > >> >>+++ b/include/linux/module.h > >> >>@@ -25,9 +25,6 @@ > >> >> #include > >> >> #include > >> >> > >> >>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */ > >> >>-#define MODULE_SIG_STRING "~Module signature appended~\n" > >> >>- > >> > > >> > Hi Thiago, apologies for the delay. > >> > >> Hello Jessica, thanks for reviewing the patch! > >> > >> > It looks like arch/s390/kernel/machine_kexec_file.c also relies on > >> > MODULE_SIG_STRING being defined, so module_signature.h will need to be > >> > included there too, otherwise we'll run into a compilation error. > >> > >> Indeed. Thanks for spotting that. The patch below fixes it. It's > >> identical to the previous version except for the changes in > >> arch/s390/kernel/machine_kexec_file.c and their description in the > >> commit message. I'm also copying some s390 people in this email. > > > > to me the s390 part looks good but for one minor nit. > > Thanks for the prompt review! > > > In arch/s390/Kconfig KEXEC_VERIFY_SIG currently depends on > > SYSTEM_DATA_VERIFICATION. I'd prefer when you update this to the new > > MODULE_SIG_FORMAT. It shouldn't make any difference right now, as we don't > > use mod_check_sig in our code path. But it could cause problems in the > > future, > > when more code might be shared. > > Makes sense. Here is the updated patch with the Kconfig change. > The patch looks good now. Thanks a lot PHilipp
Re: [PATCH v12 01/11] MODSIGN: Export module signature definitions
Hi Thiago, On Thu, 04 Jul 2019 03:42:57 -0300 Thiago Jung Bauermann wrote: > Jessica Yu writes: > > > +++ Thiago Jung Bauermann [27/06/19 23:19 -0300]: > >>IMA will use the module_signature format for append signatures, so export > >>the relevant definitions and factor out the code which verifies that the > >>appended signature trailer is valid. > >> > >>Also, create a CONFIG_MODULE_SIG_FORMAT option so that IMA can select it > >>and be able to use mod_check_sig() without having to depend on either > >>CONFIG_MODULE_SIG or CONFIG_MODULES. > >> > >>Signed-off-by: Thiago Jung Bauermann > >>Reviewed-by: Mimi Zohar > >>Cc: Jessica Yu > >>--- > >> include/linux/module.h | 3 -- > >> include/linux/module_signature.h | 44 + > >> init/Kconfig | 6 +++- > >> kernel/Makefile | 1 + > >> kernel/module.c | 1 + > >> kernel/module_signature.c| 46 ++ > >> kernel/module_signing.c | 56 +--- > >> scripts/Makefile | 2 +- > >> 8 files changed, 106 insertions(+), 53 deletions(-) > >> > >>diff --git a/include/linux/module.h b/include/linux/module.h > >>index 188998d3dca9..aa56f531cf1e 100644 > >>--- a/include/linux/module.h > >>+++ b/include/linux/module.h > >>@@ -25,9 +25,6 @@ > >> #include > >> #include > >> > >>-/* In stripped ARM and x86-64 modules, ~ is surprisingly rare. */ > >>-#define MODULE_SIG_STRING "~Module signature appended~\n" > >>- > > > > Hi Thiago, apologies for the delay. > > Hello Jessica, thanks for reviewing the patch! > > > It looks like arch/s390/kernel/machine_kexec_file.c also relies on > > MODULE_SIG_STRING being defined, so module_signature.h will need to be > > included there too, otherwise we'll run into a compilation error. > > Indeed. Thanks for spotting that. The patch below fixes it. It's > identical to the previous version except for the changes in > arch/s390/kernel/machine_kexec_file.c and their description in the > commit message. I'm also copying some s390 people in this email. to me the s390 part looks good but for one minor nit. In arch/s390/Kconfig KEXEC_VERIFY_SIG currently depends on SYSTEM_DATA_VERIFICATION. I'd prefer when you update this to the new MODULE_SIG_FORMAT. It shouldn't make any difference right now, as we don't use mod_check_sig in our code path. But it could cause problems in the future, when more code might be shared. Thanks Philipp > > Other than that, the module-related changes look good to me: > > > > Acked-by: Jessica Yu > > Thank you very much! >
Re: [PATCH RFC] generic ELF support for kexec
Hi Sven,
On Tue, 25 Jun 2019 20:54:34 +0200
Sven Schnelle wrote:
> Hi List,
>
> i recently started working on kexec for PA-RISC. While doing so, i figured
> that powerpc already has support for reading ELF images inside of the Kernel.
> My first attempt was to steal the source code and modify it for PA-RISC, but
> it turned out that i didn't had to change much. Only ARM specific stuff like
> fdt blob fetching had to be removed.
>
> So instead of duplicating the code, i thought about moving the ELF stuff to
> the core kexec code, and exposing several function to use that code from the
> arch specific code.
That's always the right approach. Well done.
> I'm attaching the patch to this Mail. What do you think about that change?
> s390 also uses ELF files, and (maybe?) could also switch to this
> implementation.
> But i don't know anything about S/390 and don't have one in my basement. So
> i'll leave s390 to the IBM folks.
I'm afraid there isn't much code here s390 can reuse. I see multiple problems
in kexec_elf_load:
* while loading the phdrs we also need to setup some data structures to pass
to the next kernel
* the s390 kernel needs to be loaded to a fixed address
* there is no support to load a crash kernel
Of course that could all be fixed/worked around by introducing some arch hooks.
But when you take into account that the whole elf loader on s390 is ~100 lines
of code, I don't think it is worth it.
More comments below.
[...]
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index b9b1bc5f9669..49b23b425f84 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -216,6 +216,41 @@ extern int crash_prepare_elf64_headers(struct crash_mem
> *mem, int kernel_map,
> void **addr, unsigned long *sz);
> #endif /* CONFIG_KEXEC_FILE */
>
> +#ifdef CONFIG_KEXEC_FILE_ELF
> +
> +struct kexec_elf_info {
> + /*
> + * Where the ELF binary contents are kept.
> + * Memory managed by the user of the struct.
> + */
> + const char *buffer;
> +
> + const struct elfhdr *ehdr;
> + const struct elf_phdr *proghdrs;
> + struct elf_shdr *sechdrs;
> +};
Do i understand this right? elf_info->buffer contains the full elf file and
elf_info->ehdr is a (endianness translated) copy of the files ehdr?
If so ...
> +void kexec_free_elf_info(struct kexec_elf_info *elf_info);
> +
> +int kexec_build_elf_info(const char *buf, size_t len, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info);
> +
> +int kexec_elf_kernel_load(struct kimage *image, struct kexec_buf *kbuf,
> + char *kernel_buf, unsigned long kernel_len,
> + unsigned long *kernel_load_addr);
> +
> +int kexec_elf_probe(const char *buf, unsigned long len);
> +
> +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info,
> + struct kexec_buf *kbuf,
> + unsigned long *lowest_load_addr);
> +
> +int kexec_elf_load(struct kimage *image, struct elfhdr *ehdr,
> + struct kexec_elf_info *elf_info,
> + struct kexec_buf *kbuf,
> + unsigned long *lowest_load_addr);
... you could simplify the arguments by dropping the ehdr argument. The
elf_info should contain all the information needed. Furthermore the kexec_buf
also contains a pointer to its kimage. So you can drop that argument as well.
An other thing is that you kzalloc the memory needed for proghdrs and sechdrs
but expect the user of those functions to provide the memory needed for ehdr.
Wouldn't it be more consistent to also kzalloc the ehdr?
[...]
> diff --git a/kernel/kexec_file_elf.c b/kernel/kexec_file_elf.c
> new file mode 100644
> index ..bb966c93492c
> --- /dev/null
> +++ b/kernel/kexec_file_elf.c
> @@ -0,0 +1,574 @@
[...]
> +static uint64_t elf64_to_cpu(const struct elfhdr *ehdr, uint64_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le64_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be64_to_cpu(value);
> +
> + return value;
> +}
> +
> +static uint16_t elf16_to_cpu(const struct elfhdr *ehdr, uint16_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le16_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be16_to_cpu(value);
> +
> + return value;
> +}
> +
> +static uint32_t elf32_to_cpu(const struct elfhdr *ehdr, uint32_t value)
> +{
> + if (ehdr->e_ident[EI_DATA] == ELFDATA2LSB)
> + value = le32_to_cpu(value);
> + else if (ehdr->e_ident[EI_DATA] == ELFDATA2MSB)
> + value = be32_to_cpu(value);
> +
> + return value;
> +}
What are the elf*_to_cpu good for? In general I'd assume that kexec loads a
kernel for the same architecture it is running on. So the
