Re: [PATCH qemu v3] x86: don't let decompressed kernel image clobber setup_data

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Jan 23, 2023 at 06:37:21AM -0600, Jason A. Donenfeld wrote:
> On Mon, Jan 23, 2023 at 6:12 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Jan 22, 2023 at 08:21:30PM -0800, Eric Biggers wrote:
> > > Hi Michael,
> > >
> > > On Tue, Jan 10, 2023 at 12:50:42PM -0500, Michael S. Tsirkin wrote:
> > > > On Tue, Jan 10, 2023 at 04:34:49PM +0100, Jason A. Donenfeld wrote:
> > > > > Hi Michael,
> > > > >
> > > > > Could you queue up this patch and mark it as a fix for 7.2.1? It is a
> > > > > straight-up bug fix for a 7.2 regression that's now affected several
> > > > > users.
> > > >
> > > > OK. In the future pls cc me if you want me to merge a patch. Thanks!
> > > >
> > > > > - It has two Tested-by tags on the thread.
> > > > > - hpa, the maintainer of the kernel side of this, confirmed on one of
> > > > >   the various tributary threads that this approach is a correct one.
> > > > > - It doesn't introduce any new functionality.
> > > > >
> > > > > For your convenience, you can grab this out of lore here:
> > > > >
> > > > >   https://lore.kernel.org/lkml/20221230220725.618763-1-Jason@zx2c4.com/
> > > > >
> > > > > Or if you want to yolo it:
> > > > >
> > > > >   curl https://lore.kernel.org/lkml/20221230220725.618763-1-Jason@zx2c4.com/raw | git am -s
> > > > >
> > > > > It's now sat silent on the mailing list for a while. So let's please get
> > > > > this committed and backported so that the bug reports stop coming in.
> > > > >
> > >
> > > This patch still isn't on QEMU's master branch.  What happened to it?
> > >
> > > - Eric
> >
> > Indeed though I remember picking it up. Tagged again now. Thanks!
> 
> Thanks. What branch is this in? I didn't see it on:
> https://gitlab.com/mstredhat/qemu/-/branches/active
> https://github.com/mstsirkin/qemu/branches

I don't use github really. And it was not pushed to gitlab as I was
figuring out issues with other patches before starting CI as CI minutes
are limited.  BTW as checkpatch was unhappy I applied a fixup -
making checkpatch happier and in the process the code change a bit
smaller.  If you want to do cleanups on top be my guest but pls
make it pass checkpatch. Thanks!


commit a00d99e04c4481fca3ee2d7c40d42993b7b059c2
Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Sat Jan 28 06:08:43 2023 -0500

    fixup! x86: don't let decompressed kernel image clobber setup_data

diff --git a/hw/i386/microvm.c b/hw/i386/microvm.c
index 1b19d28c02..29f30dd6d3 100644
--- a/hw/i386/microvm.c
+++ b/hw/i386/microvm.c
@@ -378,7 +378,7 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
     MicrovmMachineState *mms = MICROVM_MACHINE(machine);
     BusState *bus;
     BusChild *kid;
-    char *cmdline, *existing_cmdline = fw_cfg_read_bytes_ptr(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA);
+    char *cmdline, *existing_cmdline;
     size_t len;
 
     /*
@@ -388,6 +388,7 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
      * Yes, this is a hack, but one that heavily improves the UX without
      * introducing any significant issues.
      */
+    existing_cmdline = fw_cfg_read_bytes_ptr(x86ms->fw_cfg, FW_CFG_CMDLINE_DATA);
     cmdline = g_strdup(existing_cmdline);
     bus = sysbus_get_default();
     QTAILQ_FOREACH(kid, &bus->children, sibling) {
@@ -413,10 +414,11 @@ static void microvm_fix_kernel_cmdline(MachineState *machine)
     }
 
     len = strlen(cmdline);
-    if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline))
+    if (len > VIRTIO_CMDLINE_TOTAL_MAX_LEN + strlen(existing_cmdline)) {
         fprintf(stderr, "qemu: virtio mmio cmdline too large, skipping\n");
-    else
+    } else {
         memcpy(existing_cmdline, cmdline, len + 1);
+    }
     g_free(cmdline);
 }
 
diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index b57a993596..eaff4227bd 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -803,7 +803,7 @@ void x86_load_linux(X86MachineState *x86ms,
     bool linuxboot_dma_enabled = X86_MACHINE_GET_CLASS(x86ms)->fwcfg_dma_enabled;
     uint16_t protocol;
     int setup_size, kernel_size, cmdline_size;
-    int dtb_size;
+    int dtb_size, setup_data_offset;
     uint32_t initrd_max;
     uint8_t header[8192], *setup, *kernel;
     hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0, first_setup_data = 0;
@@ -818,8 +818,10 @@ void x86_load_linux(X86MachineState *x86ms,
     SevKernelLoaderContext sev_load_ctx = {};
     enum { RNG_SEED_LENGTH = 32 };
 
-    /* Add the NUL terminator, some padding for the microvm cmdline fiddling
-     * hack, and then align to 16 bytes as a paranoia measure */
+    /*
+     * Add the NUL terminator, some padding for the microvm cmdline fiddling
+     * hack, and then align to 16 bytes as a paranoia measure
+     */
     cmdline_size = (strlen(machine->kernel_cmdline) + 1 +
                     VIRTIO_CMDLINE_TOTAL_MAX_LEN + 16) & ~15;
     /* Make a copy, since we might append arbitrary bytes to it later. */
@@ -1090,22 +1092,24 @@ void x86_load_linux(X86MachineState *x86ms,
             exit(1);
         }
 
+        setup_data_offset = cmdline_size;
         cmdline_size += sizeof(SetupData) + dtb_size;
         kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size);
-        setup_data = (void *)kernel_cmdline + cmdline_size - (sizeof(SetupData) + dtb_size);
+        setup_data = (void *)kernel_cmdline + setup_data_offset;
         setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = cmdline_addr + ((void *)setup_data - (void *)kernel_cmdline);
+        first_setup_data = cmdline_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_DTB);
         setup_data->len = cpu_to_le32(dtb_size);
         load_image_size(dtb_filename, setup_data->data, dtb_size);
     }
 
     if (!legacy_no_rng_seed && protocol >= 0x209) {
+        setup_data_offset = cmdline_size;
         cmdline_size += sizeof(SetupData) + RNG_SEED_LENGTH;
         kernel_cmdline = g_realloc(kernel_cmdline, cmdline_size);
-        setup_data = (void *)kernel_cmdline + cmdline_size - (sizeof(SetupData) + RNG_SEED_LENGTH);
+        setup_data = (void *)kernel_cmdline + setup_data_offset;
         setup_data->next = cpu_to_le64(first_setup_data);
-        first_setup_data = cmdline_addr + ((void *)setup_data - (void *)kernel_cmdline);
+        first_setup_data = cmdline_addr + setup_data_offset;
         setup_data->type = cpu_to_le32(SETUP_RNG_SEED);
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);