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@xxxxxxxxxx> > > > Cc: Catalin Marinas <catalin.marinas@xxxxxxx> > > > Cc: Will Deacon <will.deacon@xxxxxxx> > > > Reviewed-by: James Morse <james.morse@xxxxxxx> > > > --- > > > 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. _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec
