| Message ID | 20200605104907.1307967-1-maz@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvmtool] arm64: Obtain text offset from kernel image | expand |
Hi Marc, On 6/5/20 11:49 AM, Marc Zyngier wrote: > Recent changes made to Linux 5.8 have outlined that kvmtool > hardcodes the text offset instead of reading it from the arm64 > image itself. > > To address this, import the image header structure into kvmtool > and do the right thing. 32bit guests are still loaded to their > usual locations. > > Reported-by: Ard Biesheuvel <ardb@kernel.org> > Signed-off-by: Marc Zyngier <maz@kernel.org> > --- > Makefile | 1 + > arm/aarch32/include/kvm/kvm-arch.h | 2 +- > arm/aarch64/include/asm/image.h | 59 ++++++++++++++++++++++++++++++ > arm/aarch64/include/kvm/kvm-arch.h | 5 +-- > arm/aarch64/kvm.c | 30 +++++++++++++++ > arm/kvm.c | 2 +- > 6 files changed, 94 insertions(+), 5 deletions(-) > create mode 100644 arm/aarch64/include/asm/image.h > create mode 100644 arm/aarch64/kvm.c > > [..] This is a great addition to kvmtool, thank you! Before I do a more in-depth review, I have some general questions. Regarding the actual value of text_offset, the booting.rst document says: "Prior to v3.17, the endianness of text_offset was not specified. In these cases image_size is zero and text_offset is 0x80000 in the endianness of the kernel. Where image_size is non-zero image_size is little-endian and must be respected. Where image_size is zero, text_offset can be assumed to be 0x80000". All header fields are declared little-endian, which looks to me like it would break kernels older than 3.17. If that was intentional, I think it's worth documenting somewhere, or at least a comment for the kvm__arch_get_kern_offset function. Now that we are parsing the kernel header, have you considered checking the magic number to make sure the user specified a valid kernel image? It might save someone some time debugging why the kernel isn't booting, if, for example, they are booting an armv7 kernel, but they forgot to specify --aarch32. Thanks, Alex
Hi Alex, On 2020-06-05 13:16, Alexandru Elisei wrote: > Hi Marc, > > On 6/5/20 11:49 AM, Marc Zyngier wrote: >> Recent changes made to Linux 5.8 have outlined that kvmtool >> hardcodes the text offset instead of reading it from the arm64 >> image itself. >> >> To address this, import the image header structure into kvmtool >> and do the right thing. 32bit guests are still loaded to their >> usual locations. >> >> Reported-by: Ard Biesheuvel <ardb@kernel.org> >> Signed-off-by: Marc Zyngier <maz@kernel.org> >> --- >> Makefile | 1 + >> arm/aarch32/include/kvm/kvm-arch.h | 2 +- >> arm/aarch64/include/asm/image.h | 59 >> ++++++++++++++++++++++++++++++ >> arm/aarch64/include/kvm/kvm-arch.h | 5 +-- >> arm/aarch64/kvm.c | 30 +++++++++++++++ >> arm/kvm.c | 2 +- >> 6 files changed, 94 insertions(+), 5 deletions(-) >> create mode 100644 arm/aarch64/include/asm/image.h >> create mode 100644 arm/aarch64/kvm.c >> >> [..] > > This is a great addition to kvmtool, thank you! Before I do a more > in-depth > review, I have some general questions. > > Regarding the actual value of text_offset, the booting.rst document > says: > > "Prior to v3.17, the endianness of text_offset was not specified. In > these cases > image_size is zero and text_offset is 0x80000 in the endianness of the > kernel. > Where image_size is non-zero image_size is little-endian and must be > respected. > Where image_size is zero, text_offset can be assumed to be 0x80000". > > All header fields are declared little-endian, which looks to me like it > would > break kernels older than 3.17. If that was intentional, I think it's > worth > documenting somewhere, or at least a comment for the > kvm__arch_get_kern_offset > function. TBH, I didn't give pre-3.17 *big-endian* much thought. Happy to document it though. > > Now that we are parsing the kernel header, have you considered > checking the magic > number to make sure the user specified a valid kernel image? It might > save someone > some time debugging why the kernel isn't booting, if, for example, they > are > booting an armv7 kernel, but they forgot to specify --aarch32. That'd be interesting. I'd be reluctant to prevent it from booting altogether, but I can definitely detect the lack of signature and print a warning. Thanks, M.
diff --git a/Makefile b/Makefile index d27ff38..35bb118 100644 --- a/Makefile +++ b/Makefile @@ -179,6 +179,7 @@ ifeq ($(ARCH), arm64) OBJS += $(OBJS_ARM_COMMON) OBJS += arm/aarch64/arm-cpu.o OBJS += arm/aarch64/kvm-cpu.o + OBJS += arm/aarch64/kvm.o ARCH_INCLUDE := $(HDRS_ARM_COMMON) ARCH_INCLUDE += -Iarm/aarch64/include diff --git a/arm/aarch32/include/kvm/kvm-arch.h b/arm/aarch32/include/kvm/kvm-arch.h index cd31e72..a772bb1 100644 --- a/arm/aarch32/include/kvm/kvm-arch.h +++ b/arm/aarch32/include/kvm/kvm-arch.h @@ -1,7 +1,7 @@ #ifndef KVM__KVM_ARCH_H #define KVM__KVM_ARCH_H -#define ARM_KERN_OFFSET(...) 0x8000 +#define kvm__arch_get_kern_offset(...) 0x8000 #define ARM_MAX_MEMORY(...) ARM_LOMAP_MAX_MEMORY diff --git a/arm/aarch64/include/asm/image.h b/arm/aarch64/include/asm/image.h new file mode 100644 index 0000000..c2b1321 --- /dev/null +++ b/arm/aarch64/include/asm/image.h @@ -0,0 +1,59 @@ +/* SPDX-License-Identifier: GPL-2.0 */ + +#ifndef __ASM_IMAGE_H +#define __ASM_IMAGE_H + +#define ARM64_IMAGE_MAGIC "ARM\x64" + +#define ARM64_IMAGE_FLAG_BE_SHIFT 0 +#define ARM64_IMAGE_FLAG_PAGE_SIZE_SHIFT (ARM64_IMAGE_FLAG_BE_SHIFT + 1) +#define ARM64_IMAGE_FLAG_PHYS_BASE_SHIFT \ + (ARM64_IMAGE_FLAG_PAGE_SIZE_SHIFT + 2) +#define ARM64_IMAGE_FLAG_BE_MASK 0x1 +#define ARM64_IMAGE_FLAG_PAGE_SIZE_MASK 0x3 +#define ARM64_IMAGE_FLAG_PHYS_BASE_MASK 0x1 + +#define ARM64_IMAGE_FLAG_LE 0 +#define ARM64_IMAGE_FLAG_BE 1 +#define ARM64_IMAGE_FLAG_PAGE_SIZE_4K 1 +#define ARM64_IMAGE_FLAG_PAGE_SIZE_16K 2 +#define ARM64_IMAGE_FLAG_PAGE_SIZE_64K 3 +#define ARM64_IMAGE_FLAG_PHYS_BASE 1 + +#ifndef __ASSEMBLY__ + +#define arm64_image_flag_field(flags, field) \ + (((flags) >> field##_SHIFT) & field##_MASK) + +/* + * struct arm64_image_header - arm64 kernel image header + * See Documentation/arm64/booting.rst 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 { + __le32 code0; + __le32 code1; + __le64 text_offset; + __le64 image_size; + __le64 flags; + __le64 res2; + __le64 res3; + __le64 res4; + __le32 magic; + __le32 res5; +}; + +#endif /* __ASSEMBLY__ */ + +#endif /* __ASM_IMAGE_H */ diff --git a/arm/aarch64/include/kvm/kvm-arch.h b/arm/aarch64/include/kvm/kvm-arch.h index 9de623a..55ef8ed 100644 --- a/arm/aarch64/include/kvm/kvm-arch.h +++ b/arm/aarch64/include/kvm/kvm-arch.h @@ -1,9 +1,8 @@ #ifndef KVM__KVM_ARCH_H #define KVM__KVM_ARCH_H -#define ARM_KERN_OFFSET(kvm) ((kvm)->cfg.arch.aarch32_guest ? \ - 0x8000 : \ - 0x80000) +struct kvm; +unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd); #define ARM_MAX_MEMORY(kvm) ((kvm)->cfg.arch.aarch32_guest ? \ ARM_LOMAP_MAX_MEMORY : \ diff --git a/arm/aarch64/kvm.c b/arm/aarch64/kvm.c new file mode 100644 index 0000000..ed23ee9 --- /dev/null +++ b/arm/aarch64/kvm.c @@ -0,0 +1,30 @@ +#include "kvm/kvm.h" + +#include <asm/image.h> + +#include <linux/byteorder.h> + +unsigned long long kvm__arch_get_kern_offset(struct kvm *kvm, int fd) +{ + struct arm64_image_header header; + off_t cur_offset; + ssize_t size; + + /* the 32bit kernel offset is a well known value */ + if (kvm->cfg.arch.aarch32_guest) + return 0x8000; + + cur_offset = lseek(fd, 0, SEEK_CUR); + if (cur_offset == (off_t)-1 || + lseek(fd, 0, SEEK_SET) == (off_t)-1) + die("Failed to seek in image file"); + + size = xread(fd, &header, sizeof(header)); + if (size < 0 || (size_t)size < sizeof(header)) + die("Failed to read kernel image header"); + + lseek(fd, cur_offset, SEEK_SET); + + return le64_to_cpu(header.text_offset); +} + diff --git a/arm/kvm.c b/arm/kvm.c index 1f85fc6..5aea18f 100644 --- a/arm/kvm.c +++ b/arm/kvm.c @@ -103,7 +103,7 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd, */ limit = kvm->ram_start + min(kvm->ram_size, (u64)SZ_256M) - 1; - pos = kvm->ram_start + ARM_KERN_OFFSET(kvm); + pos = kvm->ram_start + kvm__arch_get_kern_offset(kvm, fd_kernel); kvm->arch.kern_guest_start = host_to_guest_flat(kvm, pos); file_size = read_file(fd_kernel, pos, limit - pos); if (file_size < 0) {
Recent changes made to Linux 5.8 have outlined that kvmtool hardcodes the text offset instead of reading it from the arm64 image itself. To address this, import the image header structure into kvmtool and do the right thing. 32bit guests are still loaded to their usual locations. Reported-by: Ard Biesheuvel <ardb@kernel.org> Signed-off-by: Marc Zyngier <maz@kernel.org> --- Makefile | 1 + arm/aarch32/include/kvm/kvm-arch.h | 2 +- arm/aarch64/include/asm/image.h | 59 ++++++++++++++++++++++++++++++ arm/aarch64/include/kvm/kvm-arch.h | 5 +-- arm/aarch64/kvm.c | 30 +++++++++++++++ arm/kvm.c | 2 +- 6 files changed, 94 insertions(+), 5 deletions(-) create mode 100644 arm/aarch64/include/asm/image.h create mode 100644 arm/aarch64/kvm.c