| Message ID | 20241128151246.10858-3-alexandru.elisei@arm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm: Payload memory layout change | expand |
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@arm.com> > --- > 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 >
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"); 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.");
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@arm.com> --- arm/kvm.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)