On Thu, Nov 28, 2024 at 03:12:44PM +0000, Alexandru Elisei wrote:
> kvmtool, on arm and arm64, puts the kernel, DTB and initrd (if present) in
> a 256MB memory region that starts at the bottom of RAM.
> kvm__arch_load_kernel_image() copies the kernel at the start of RAM, the
> DTB is placed at the top of the region, and immediately below it the
> initrd.
>
> When the initrd is specified by the user, kvmtool checks that it doesn't
> overlap with the kernel by computing the start address in the host's
> address space:
>
> fstat(fd_initrd, &sb);
> pos = pos - (sb.st_size + INITRD_ALIGN);
> guest_addr = ALIGN(host_to_guest_flat(kvm, pos), INITRD_ALIGN); (a)
> pos = guest_flat_to_host(kvm, guest_addr); (b)
>
> If the initrd is large enough to completely overwrite the kernel and start
> below the guest RAM (pos < kvm->ram_start), then kvmtool will omit the
> following errors:
>
> Warning: unable to translate host address 0xfffe849ffffc to guest (1)
> Warning: unable to translate guest address 0x0 to host (2)
> Fatal: initrd overlaps with kernel image. (3)
>
> (1) is because (a) calls host_to_guest_flat(kvm, pos) with a 'pos'
> outside any of the memslots.
>
> (2) is because guest_flat_to_host() is called at (b) with guest_addr=0,
> which is what host_to_guest_flat() returns if the supplied address is not
> found in any of the memslots. This warning is eliminated by this patch.
>
> And finally, (3) is the most useful message, because it tells the user what
> the error is.
>
> The issue is a more general pattern in kvm__arch_load_kernel_image():
> kvmtool doesn't check if host_to_guest_flat() returns 0, which means that
> the host address is not within any of the memslots. Add a check for that,
> which will at the very least remove the second warning.
>
> This also fixes the following edge cases:
>
> 1. The same warnings being emitted in a similar scenario with the DTB, when
> the RAM is smaller than FDT_MAX_SIZE + FDT_ALIGN.
>
> 2. When copying the kernel, if the RAM size is smaller than the kernel
> offset, the start of the kernel (represented by the variable 'pos') will be
> outside the VA space allocated for the guest RAM. limit - pos will wrap
> around, because gcc 14.1.1 wraps the pointers (void pointer arithmetic is
> undefined in C99). Then read_file()->..->read() will return -EFAULT because
> the destination address is unallocated (as per man 2 read, also reproduced
> during testing).
>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> ---
> arm/kvm.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/arm/kvm.c b/arm/kvm.c
> index da0430c40c36..4beae69e1fb3 100644
> --- a/arm/kvm.c
> +++ b/arm/kvm.c
> @@ -113,6 +113,8 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
>
> pos = kvm->ram_start + kvm__arch_get_kern_offset(kvm, fd_kernel);
> kvm->arch.kern_guest_start = host_to_guest_flat(kvm, pos);
> + if (!kvm->arch.kern_guest_start)
> + die("guest memory too small to contain the kernel");
Just doing a quick drive-by and noticed this indentation issue.
Thanks,
drew
> file_size = read_file(fd_kernel, pos, limit - pos);
> if (file_size < 0) {
> if (errno == ENOMEM)
> @@ -131,7 +133,10 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
> */
> pos = limit;
> pos -= (FDT_MAX_SIZE + FDT_ALIGN);
> - guest_addr = ALIGN(host_to_guest_flat(kvm, pos), FDT_ALIGN);
> + guest_addr = host_to_guest_flat(kvm, pos);
> + if (!guest_addr)
> + die("fdt too big to contain in guest memory");
> + guest_addr = ALIGN(guest_addr, FDT_ALIGN);
> pos = guest_flat_to_host(kvm, guest_addr);
> if (pos < kernel_end)
> die("fdt overlaps with kernel image.");
> @@ -151,7 +156,10 @@ bool kvm__arch_load_kernel_image(struct kvm *kvm, int fd_kernel, int fd_initrd,
> die_perror("fstat");
>
> pos -= (sb.st_size + INITRD_ALIGN);
> - guest_addr = ALIGN(host_to_guest_flat(kvm, pos), INITRD_ALIGN);
> + guest_addr = host_to_guest_flat(kvm, pos);
> + if (!guest_addr)
> + die("initrd too big to fit in the payload memory region");
> + guest_addr = ALIGN(guest_addr, INITRD_ALIGN);
> pos = guest_flat_to_host(kvm, guest_addr);
> if (pos < kernel_end)
> die("initrd overlaps with kernel image.");
> --
> 2.47.0
>
