| Message ID | 20180425062629.29404-7-takahiro.akashi@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Akashi, On 25/04/18 07:26, 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. > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h > index e4de1223715f..3cba4161818a 100644 > --- a/arch/arm64/include/asm/kexec.h > +++ b/arch/arm64/include/asm/kexec.h > @@ -102,6 +102,56 @@ struct kimage_arch { > void *dtb_buf; > }; > > +/** > + * struct arm64_image_header - arm64 kernel image header > + * > + * @pe_sig: Optional PE format 'MZ' signature > + * @branch_code: Instruction to branch to stext > + * @text_offset: Image load offset, little endian > + * @image_size: Effective image size, little endian > + * @flags: > + * Bit 0: Kernel endianness. 0=little endian, 1=big endian Page size? What about 'phys_base'?, (whatever that is...) Probably best to refer to Documentation/arm64/booting.txt here, its the authoritative source of what these fields mean. > + * @reserved: Reserved > + * @magic: Magic number, "ARM\x64" > + * @pe_header: Optional offset to a PE format header > + **/ > + > +struct arm64_image_header { > + u8 pe_sig[2]; > + u8 pad[2]; > + u32 branch_code; > + u64 text_offset; > + u64 image_size; > + u64 flags; __le64 as appropriate here would let tools like sparse catch any missing endian conversion bugs. > + u64 reserved[3]; > + u8 magic[4]; > + u32 pe_header; > +}; I'm surprised we don't have a definition for this already, I guess its always done in asm. We have kernel/image.h that holds some of this stuff, if we are going to validate the flags, is it worth adding the code there, (and moving it to include/asm)? > +static const u8 arm64_image_magic[4] = {'A', 'R', 'M', 0x64U}; Any chance this magic could be a pre-processor symbol shared with head.S? > + > +/** > + * arm64_header_check_magic - Helper to check the arm64 image header. > + * > + * Returns non-zero if header is OK. > + */ > + > +static inline int arm64_header_check_magic(const struct arm64_image_header *h) > +{ > + if (!h) > + return 0; > + > + if (!h->text_offset) > + return 0; > + > + return (h->magic[0] == arm64_image_magic[0] > + && h->magic[1] == arm64_image_magic[1] > + && h->magic[2] == arm64_image_magic[2] > + && h->magic[3] == arm64_image_magic[3]); memcmp()? Or just define it as a 32bit value? I guess you skip the MZ prefix as its not present for !EFI? Could we check branch_code is non-zero, and text-offset points within image-size? We could check that this platform supports the page-size/endian config that this Image was built with... We get a message from the EFI stub if the page-size can't be supported, it would be nice to do the same here (as we can). (no idea if kexec-tool checks this stuff, it probably can't get at the id registers to know) > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c > new file mode 100644 > index 000000000000..4dd524ad6611 > --- /dev/null > +++ b/arch/arm64/kernel/kexec_image.c > @@ -0,0 +1,79 @@ > +static void *image_load(struct kimage *image, > + char *kernel, unsigned long kernel_len, > + char *initrd, unsigned long initrd_len, > + char *cmdline, unsigned long cmdline_len) > +{ > + struct kexec_buf kbuf; > + struct arm64_image_header *h = (struct arm64_image_header *)kernel; > + unsigned long text_offset; > + int ret; > + > + /* Load the kernel */ > + kbuf.image = image; > + kbuf.buf_min = 0; > + kbuf.buf_max = ULONG_MAX; > + kbuf.top_down = false; > + > + kbuf.buffer = kernel; > + kbuf.bufsz = kernel_len; > + kbuf.memsz = le64_to_cpu(h->image_size); > + text_offset = le64_to_cpu(h->text_offset); > + kbuf.buf_align = SZ_2M; > + /* Adjust kernel segment with TEXT_OFFSET */ > + kbuf.memsz += text_offset; > + > + ret = kexec_add_buffer(&kbuf); > + if (ret) > + goto out; > + > + image->arch.kern_segment = image->nr_segments - 1; You only seem to use kern_segment here, and in load_other_segments() called below. Could it not be a local variable passed in? Instead of arch-specific data we keep forever? > + image->segment[image->arch.kern_segment].mem += text_offset; > + image->segment[image->arch.kern_segment].memsz -= text_offset; > + image->start = image->segment[image->arch.kern_segment].mem; > + > + pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > + image->segment[image->arch.kern_segment].mem, > + kbuf.bufsz, kbuf.memsz); > + > + /* Load additional data */ > + ret = load_other_segments(image, initrd, initrd_len, > + cmdline, cmdline_len); > + > +out: > + return ERR_PTR(ret); > +} Looks good, Thanks, James
James, On Tue, May 01, 2018 at 06:46:11PM +0100, James Morse wrote: > Hi Akashi, > > On 25/04/18 07:26, 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. > > > > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h > > index e4de1223715f..3cba4161818a 100644 > > --- a/arch/arm64/include/asm/kexec.h > > +++ b/arch/arm64/include/asm/kexec.h > > @@ -102,6 +102,56 @@ struct kimage_arch { > > void *dtb_buf; > > }; > > > > +/** > > + * struct arm64_image_header - arm64 kernel image header > > + * > > + * @pe_sig: Optional PE format 'MZ' signature > > + * @branch_code: Instruction to branch to stext > > + * @text_offset: Image load offset, little endian > > + * @image_size: Effective image size, little endian > > + * @flags: > > + * Bit 0: Kernel endianness. 0=little endian, 1=big endian > > Page size? What about 'phys_base'?, (whatever that is...) > Probably best to refer to Documentation/arm64/booting.txt here, its the > authoritative source of what these fields mean. While we don't care other bit fields for now, I will add the reference to the Documentation file. > > > + * @reserved: Reserved > > + * @magic: Magic number, "ARM\x64" > > + * @pe_header: Optional offset to a PE format header > > + **/ > > + > > +struct arm64_image_header { > > + u8 pe_sig[2]; > > + u8 pad[2]; > > + u32 branch_code; > > + u64 text_offset; > > + u64 image_size; > > + u64 flags; > > __le64 as appropriate here would let tools like sparse catch any missing endian > conversion bugs. OK. > > > + u64 reserved[3]; > > + u8 magic[4]; > > + u32 pe_header; > > +}; > > I'm surprised we don't have a definition for this already, I guess its always > done in asm. We have kernel/image.h that holds some of this stuff, if we are > going to validate the flags, is it worth adding the code there, (and moving it > to include/asm)? A comment at the beginning of this file says, #ifndef LINKER_SCRIPT #error This file should only be included in vmlinux.lds.S #endif Let me think about. > > > +static const u8 arm64_image_magic[4] = {'A', 'R', 'M', 0x64U}; > > Any chance this magic could be a pre-processor symbol shared with head.S? OK. > > > + > > +/** > > + * arm64_header_check_magic - Helper to check the arm64 image header. > > + * > > + * Returns non-zero if header is OK. > > + */ > > + > > +static inline int arm64_header_check_magic(const struct arm64_image_header *h) > > +{ > > + if (!h) > > + return 0; > > + > > + if (!h->text_offset) > > + return 0; > > + > > + return (h->magic[0] == arm64_image_magic[0] > > + && h->magic[1] == arm64_image_magic[1] > > + && h->magic[2] == arm64_image_magic[2] > > + && h->magic[3] == arm64_image_magic[3]); > > memcmp()? Or just define it as a 32bit value? OK. As you know, I always tried to keep the code not diverted from kexec-tools for maintainability reason. > I guess you skip the MZ prefix as its not present for !EFI? CONFIG_KEXEC_IMAGE_VERIFY_SIG depends on the fact that the file format is PE (that is, EFI is enabled). > Could we check branch_code is non-zero, and text-offset points within image-size? We could do it, but I don't think this check is very useful. > > We could check that this platform supports the page-size/endian config that this > Image was built with... We get a message from the EFI stub if the page-size > can't be supported, it would be nice to do the same here (as we can). There is no restriction on page-size or endianness for kexec. What will be the purpose of this check? > (no idea if kexec-tool checks this stuff, it probably can't get at the id > registers to know) > > > > diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c > > new file mode 100644 > > index 000000000000..4dd524ad6611 > > --- /dev/null > > +++ b/arch/arm64/kernel/kexec_image.c > > @@ -0,0 +1,79 @@ > > > +static void *image_load(struct kimage *image, > > + char *kernel, unsigned long kernel_len, > > + char *initrd, unsigned long initrd_len, > > + char *cmdline, unsigned long cmdline_len) > > +{ > > + struct kexec_buf kbuf; > > + struct arm64_image_header *h = (struct arm64_image_header *)kernel; > > + unsigned long text_offset; > > + int ret; > > + > > + /* Load the kernel */ > > + kbuf.image = image; > > + kbuf.buf_min = 0; > > + kbuf.buf_max = ULONG_MAX; > > + kbuf.top_down = false; > > + > > + kbuf.buffer = kernel; > > + kbuf.bufsz = kernel_len; > > + kbuf.memsz = le64_to_cpu(h->image_size); > > + text_offset = le64_to_cpu(h->text_offset); > > + kbuf.buf_align = SZ_2M; > > > + /* Adjust kernel segment with TEXT_OFFSET */ > > + kbuf.memsz += text_offset; > > + > > + ret = kexec_add_buffer(&kbuf); > > + if (ret) > > + goto out; > > + > > + image->arch.kern_segment = image->nr_segments - 1; > > You only seem to use kern_segment here, and in load_other_segments() called > below. Could it not be a local variable passed in? Instead of arch-specific data > we keep forever? No, kern_segment is also used in load_other_segments() in machine_kexec_file.c. To optimize memory hole allocation logic in locate_mem_hole_callback(), we need to know the exact range of kernel image (start and end). (Known drawback in this code is that Image only occupies one segment, but once vmlinux might be supported, it would occupy two segments for text and data.) > > > + image->segment[image->arch.kern_segment].mem += text_offset; > > + image->segment[image->arch.kern_segment].memsz -= text_offset; > > + image->start = image->segment[image->arch.kern_segment].mem; > > + > > + pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n", > > + image->segment[image->arch.kern_segment].mem, > > + kbuf.bufsz, kbuf.memsz); > > + > > + /* Load additional data */ > > + ret = load_other_segments(image, initrd, initrd_len, > > + cmdline, cmdline_len); > > + > > +out: > > + return ERR_PTR(ret); > > +} > Looks good, Thank you for thorough review. -Takahiro AKASHI > Thanks, > > James
diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h index e4de1223715f..3cba4161818a 100644 --- a/arch/arm64/include/asm/kexec.h +++ b/arch/arm64/include/asm/kexec.h @@ -102,6 +102,56 @@ struct kimage_arch { void *dtb_buf; }; +/** + * struct arm64_image_header - arm64 kernel image header + * + * @pe_sig: Optional PE format 'MZ' signature + * @branch_code: Instruction to branch to stext + * @text_offset: Image load offset, little endian + * @image_size: Effective image size, little endian + * @flags: + * Bit 0: Kernel endianness. 0=little endian, 1=big endian + * @reserved: Reserved + * @magic: Magic number, "ARM\x64" + * @pe_header: Optional offset to a PE format header + **/ + +struct arm64_image_header { + u8 pe_sig[2]; + u8 pad[2]; + u32 branch_code; + u64 text_offset; + u64 image_size; + u64 flags; + u64 reserved[3]; + u8 magic[4]; + u32 pe_header; +}; + +static const u8 arm64_image_magic[4] = {'A', 'R', 'M', 0x64U}; + +/** + * arm64_header_check_magic - Helper to check the arm64 image header. + * + * Returns non-zero if header is OK. + */ + +static inline int arm64_header_check_magic(const struct arm64_image_header *h) +{ + if (!h) + return 0; + + if (!h->text_offset) + return 0; + + return (h->magic[0] == arm64_image_magic[0] + && h->magic[1] == arm64_image_magic[1] + && h->magic[2] == arm64_image_magic[2] + && h->magic[3] == arm64_image_magic[3]); +} + +extern const struct kexec_file_ops kexec_image_ops; + struct kimage; extern int load_other_segments(struct kimage *image, diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 2f2b2757ae7a..1e110aa571dd 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -50,7 +50,7 @@ arm64-obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o arm64-obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o arm64-obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \ cpu-reset.o -arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o +arm64-obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o arm64-obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o arm64-obj-$(CONFIG_CRASH_DUMP) += crash_dump.o diff --git a/arch/arm64/kernel/kexec_image.c b/arch/arm64/kernel/kexec_image.c new file mode 100644 index 000000000000..4dd524ad6611 --- /dev/null +++ b/arch/arm64/kernel/kexec_image.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Kexec image loader + + * Copyright (C) 2018 Linaro Limited + * Author: AKASHI Takahiro <takahiro.akashi@linaro.org> + */ + +#define pr_fmt(fmt) "kexec_file(Image): " fmt + +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/kexec.h> +#include <asm/byteorder.h> +#include <asm/memory.h> + +static int image_probe(const char *kernel_buf, unsigned long kernel_len) +{ + const struct arm64_image_header *h; + + h = (const struct arm64_image_header *)(kernel_buf); + + if ((kernel_len < sizeof(*h)) || !arm64_header_check_magic(h)) + return -EINVAL; + + return 0; +} + +static void *image_load(struct kimage *image, + char *kernel, unsigned long kernel_len, + char *initrd, unsigned long initrd_len, + char *cmdline, unsigned long cmdline_len) +{ + struct kexec_buf kbuf; + struct arm64_image_header *h = (struct arm64_image_header *)kernel; + unsigned long text_offset; + int ret; + + /* Load the kernel */ + kbuf.image = image; + kbuf.buf_min = 0; + kbuf.buf_max = ULONG_MAX; + kbuf.top_down = false; + + kbuf.buffer = kernel; + kbuf.bufsz = kernel_len; + kbuf.memsz = le64_to_cpu(h->image_size); + text_offset = le64_to_cpu(h->text_offset); + kbuf.buf_align = SZ_2M; + + /* Adjust kernel segment with TEXT_OFFSET */ + kbuf.memsz += text_offset; + + ret = kexec_add_buffer(&kbuf); + if (ret) + goto out; + + image->arch.kern_segment = image->nr_segments - 1; + image->segment[image->arch.kern_segment].mem += text_offset; + image->segment[image->arch.kern_segment].memsz -= text_offset; + image->start = image->segment[image->arch.kern_segment].mem; + + pr_debug("Loaded kernel at 0x%lx bufsz=0x%lx memsz=0x%lx\n", + image->segment[image->arch.kern_segment].mem, + kbuf.bufsz, kbuf.memsz); + + /* Load additional data */ + ret = load_other_segments(image, initrd, initrd_len, + cmdline, cmdline_len); + +out: + return ERR_PTR(ret); +} + +const struct kexec_file_ops kexec_image_ops = { + .probe = image_probe, + .load = image_load, +}; diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c index b3b9b1725d8a..37c0a9dc2e47 100644 --- a/arch/arm64/kernel/machine_kexec_file.c +++ b/arch/arm64/kernel/machine_kexec_file.c @@ -23,6 +23,7 @@ static int __dt_root_addr_cells; static int __dt_root_size_cells; const struct kexec_file_ops * const kexec_file_loaders[] = { + &kexec_image_ops, NULL };
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@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will.deacon@arm.com> --- arch/arm64/include/asm/kexec.h | 50 ++++++++++++++++ arch/arm64/kernel/Makefile | 2 +- arch/arm64/kernel/kexec_image.c | 79 ++++++++++++++++++++++++++ arch/arm64/kernel/machine_kexec_file.c | 1 + 4 files changed, 131 insertions(+), 1 deletion(-) create mode 100644 arch/arm64/kernel/kexec_image.c