Hi Jason,
On 28/12/22 15:38, Jason A. Donenfeld wrote:
The setup_data links are appended to the compressed kernel image. Since
the kernel image is typically loaded at 0x100000, setup_data lives at
`0x100000 + compressed_size`, which does not get relocated during the
kernel's boot process.
The kernel typically decompresses the image starting at address
0x1000000 (note: there's one more zero there than the decompressed image
above). This usually is fine for most kernels.
However, if the compressed image is actually quite large, then
setup_data will live at a `0x100000 + compressed_size` that extends into
the decompressed zone at 0x1000000. In other words, if compressed_size
is larger than `0x1000000 - 0x100000`, then the decompression step will
clobber setup_data, resulting in crashes.
Fix this by detecting that possibility, and if it occurs, put setup_data
*after* the end of the decompressed kernel, so that it doesn't get
clobbered.
One caveat is that this only works for images less than around 64
megabytes, so just bail out in that case. This is unfortunate, but I
don't currently have a way of fixing it.
Cc: x...@kernel.org
Signed-off-by: Jason A. Donenfeld <ja...@zx2c4.com>
---
hw/i386/x86.c | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index 78cc131926..628fd2b2e9 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1077,6 +1077,36 @@ void x86_load_linux(X86MachineState *x86ms,
}
fclose(f);
+ /* If a setup_data is going to be used, make sure that the bootloader won't
+ * decompress into it and clobber those bytes. */
+ if (dtb_filename || !legacy_no_rng_seed) {
+ uint32_t payload_offset = ldl_p(setup + 0x248);
+ uint32_t payload_length = ldl_p(setup + 0x24c);
+ uint32_t target_address = ldl_p(setup + 0x258);
Nitpicking, can the Linux kernel add these magic values in
arch/x86/include/uapi/asm/bootparam.h? Or can we use
offsetof(setup_header) to get them?
+ uint32_t decompressed_length = ldl_p(kernel + payload_offset +
payload_length - 4);
+
+ uint32_t estimated_setup_data_length = 4096 * 16;
+ uint32_t start_setup_data = prot_addr + kernel_size;
+ uint32_t end_setup_data = start_setup_data +
estimated_setup_data_length;
+ uint32_t start_target = target_address;
+ uint32_t end_target = target_address + decompressed_length;
Maybe we can simply use 'unsigned' type.
+ if ((start_setup_data >= start_target && start_setup_data <
end_target) ||
+ (end_setup_data >= start_target && end_setup_data < end_target)) {
+ uint32_t padded_size = target_address + decompressed_length -
prot_addr;
+
+ /* The early stage can't address past around 64 MB from the
original
+ * mapping, so just give up in that case. */
+ if (padded_size < 62 * 1024 * 1024)
You mention 64 but check for 62, is that expected? You can use the MiB
definitions to ease code review: 64 * MiB.
+ kernel_size = padded_size;
+ else {
+ fprintf(stderr, "qemu: Kernel image too large to hold
setup_data\n");
+ dtb_filename = NULL;
+ legacy_no_rng_seed = true;
+ }
+ }
+ }
Fix looks good, glad you figured out the problem.
Regards,
Phil.
