| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 10 Oct 2018 10:47:51 +0100
From: Mark Rutland <mark.rutland@....com>
To: AKASHI Takahiro <takahiro.akashi@...aro.org>,
catalin.marinas@....com, will.deacon@....com, dhowells@...hat.com,
vgoyal@...hat.com, herbert@...dor.apana.org.au,
davem@...emloft.net, dyoung@...hat.com, bhe@...hat.com,
arnd@...db.de, schwidefsky@...ibm.com, heiko.carstens@...ibm.com,
prudo@...ux.ibm.com, ard.biesheuvel@...aro.org,
james.morse@....com, bhsharma@...hat.com,
kexec@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v15 11/16] arm64: kexec_file: allow for loading
Image-format kernel
On Wed, Oct 10, 2018 at 03:52:37PM +0900, AKASHI Takahiro wrote:
> Mark,
>
> On Tue, Oct 09, 2018 at 04:28:00PM +0100, Mark Rutland wrote:
> > On Fri, Sep 28, 2018 at 03:48:36PM +0900, AKASHI Takahiro wrote:
> > > This patch provides kexec_file_ops for "Image"-format kernel. In this
> > > implementation, a binary is always loaded with a fixed offset identified
> > > in text_offset field of its header.
> > >
> > > Regarding signature verification for trusted boot, this patch doesn't
> > > contains CONFIG_KEXEC_VERIFY_SIG support, which is to be added later
> > > in this series, but file-attribute-based verification is still a viable
> > > option by enabling IMA security subsystem.
> > >
> > > You can sign(label) a to-be-kexec'ed kernel image on target file system
> > > with:
> > > $ evmctl ima_sign --key /path/to/private_key.pem Image
> > >
> > > On live system, you must have IMA enforced with, at least, the following
> > > security policy:
> > > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig"
> > >
> > > See more details about IMA here:
> > > https://sourceforge.net/p/linux-ima/wiki/Home/
> > >
> > > Signed-off-by: AKASHI Takahiro <takahiro.akashi@...aro.org>
> > > Cc: Catalin Marinas <catalin.marinas@....com>
> > > Cc: Will Deacon <will.deacon@....com>
> > > Reviewed-by: James Morse <james.morse@....com>
> > > ---
> > > arch/arm64/include/asm/kexec.h | 28 +++++++
> > > arch/arm64/kernel/Makefile | 2 +-
> > > arch/arm64/kernel/kexec_image.c | 108 +++++++++++++++++++++++++
> > > arch/arm64/kernel/machine_kexec_file.c | 1 +
> > > 4 files changed, 138 insertions(+), 1 deletion(-)
> > > create mode 100644 arch/arm64/kernel/kexec_image.c
> > >
> > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> > > index 157b2897d911..5e673481b3a3 100644
> > > --- a/arch/arm64/include/asm/kexec.h
> > > +++ b/arch/arm64/include/asm/kexec.h
> > > @@ -101,6 +101,34 @@ struct kimage_arch {
> > > unsigned long dtb_mem;
> > > };
> > >
> > > +/**
> > > + * struct arm64_image_header - arm64 kernel image header
> > > + * See Documentation/arm64/booting.txt for details
> > > + *
> > > + * @mz_magic: DOS header magic number ('MZ', optional)
> >
> > Please just call this code0. If CONFIG_EFI is disabled, it is not 'MZ'.
>
> How about this?
> (This definition will go into a new header, asm/image.h.)
>
> ---8<---
> /*
> * struct arm64_image_header - arm64 kernel image header
> * See Documentation/arm64/booting.txt for details
> *
> * @code0: Executable code, or
> * @mz_header alternatively used for part of MZ header
> * @code1: Executable code
> * @text_offset: Image load offset
> * @image_size: Effective Image size
> * @flags: kernel flags
> * @reserved: reserved
> * @magic: Magic number
> * @reserved5: reserved, or
> * @pe_header: alternatively used for PE COFF offset
> */
>
> struct arm64_image_header {
> union {
> __le32 code0;
> struct {
> __le16 magic;
> __le16 pad;
> } mz_header;
> };
> __le32 code1;
> __le64 text_offset;
> __le64 image_size;
> __le64 flags;
> __le64 reserved[3];
> __le32 magic;
> union {
> __le32 reserved5;
> __le32 pe_header;
> };
> };
Do we care about the MZ header?
The definition of the Image header in Documentation/arm64/booting.txt is:
u32 code0; /* Executable code */
u32 code1; /* Executable code */
u64 text_offset; /* Image load offset, little endian */
u64 image_size; /* Effective Image size, little endian */
u64 flags; /* kernel flags, little endian */
u64 res2 = 0; /* reserved */
u64 res3 = 0; /* reserved */
u64 res4 = 0; /* reserved */
u32 magic = 0x644d5241; /* Magic number, little endian, "ARM\x64" */
u32 res5; /* reserved (used for PE COFF offset) */
I'd prefer that we aligned our header definition with that, rather than
diverging from it.
If we need to look at the MZ magic to determine if the Image is a valid PE
binary, can't we just cast to the existing struct mz_hdr from <linux/pe.h> to
do that?
[...]
> > > + /* Check cpu features */
> > > + flags = le64_to_cpu(h->flags);
> > > + value = head_flag_field(flags, HEAD_FLAG_BE);
> > > + if (((value == HEAD_FLAG_BE) && !IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ||
> > > + ((value != HEAD_FLAG_BE) && IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)))
> > > + if (!system_supports_mixed_endian())
> > > + return ERR_PTR(-EINVAL);
> >
> > I think this can be simplified:
> >
> > bool be_image = head_flag_field(flags, HEAD_FLAG_BE);
> > bool be_kernel = IS_ENABLED(CONFIG_CPU_BIG_ENDIAN);
> >
> > if ((be_image != be_kernel) && !system_supports_mixed_endian)
> > return ERR_PTR(-EINVAL);
>
> Okay.
>
> > ... though do we need to police this at all? It's arguably policy given
> > the new image has to be signed anyway), and there are other fields that
> > could fall into that category in future.
>
> The aim here is to prevent any images from being loaded
> when there is no question that a new image will never be successfully
> kexec'ed since the core on a given SoC obviously doesn't support cpu
> features that are assumed and required by a image.
>
> I believe that this check is a good and easy practice to avoid possible
> failures before execution.
My only concern is that this is arguably placing some policy in the
kernel, and I don't want to set the expectation that we'll do this for
other things in future, as that becomes a maintenance nightmare.
I'm not necessarily opposed to these specific checks, given they're
simple. Just wanted to make sure that we've thought about it.
Thanks,
Mark.
Powered by blists - more mailing lists