| Message ID | 20220721100959.427518-1-Jason@zx2c4.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] hw/i386: pass RNG seed via setup_data entry | expand |
On Thu, Jul 21, 2022 at 12:09:59PM +0200, Jason A. Donenfeld wrote: > Tiny machines optimized for fast boot time generally don't use EFI, > which means a random seed has to be supplied some other way. For this > purpose, Linux (≥5.20) supports passing a seed in the setup_data table > with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and > specialized bootloaders. The linked commit shows the upstream kernel > implementation. > > Link: https://git.kernel.org/tip/tip/c/68b8e9713c8 > Cc: Michael S. Tsirkin <mst@redhat.com> > Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Eduardo Habkost <eduardo@habkost.net> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> > Cc: Laurent Vivier <laurent@vivier.eu> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> > --- > hw/i386/x86.c | 21 +++++++++++++++++--- > include/standard-headers/asm-x86/bootparam.h | 1 + > 2 files changed, 19 insertions(+), 3 deletions(-) > > diff --git a/hw/i386/x86.c b/hw/i386/x86.c > index 6003b4b2df..284c97f158 100644 > --- a/hw/i386/x86.c > +++ b/hw/i386/x86.c > @@ -26,6 +26,7 @@ > #include "qemu/cutils.h" > #include "qemu/units.h" > #include "qemu/datadir.h" > +#include "qemu/guest-random.h" > #include "qapi/error.h" > #include "qapi/qmp/qerror.h" > #include "qapi/qapi-visit-common.h" > @@ -771,7 +772,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, setup_data_offset; > + int dtb_size, setup_data_offset, last_setup_data_offset = 0; > uint32_t initrd_max; > uint8_t header[8192], *setup, *kernel; > hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0; > @@ -1063,16 +1064,30 @@ void x86_load_linux(X86MachineState *x86ms, > kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size; > kernel = g_realloc(kernel, kernel_size); > > - stq_p(header + 0x250, prot_addr + setup_data_offset); > setup_data = (struct setup_data *)(kernel + setup_data_offset); > - setup_data->next = 0; > + setup_data->next = last_setup_data_offset; does this make any difference? if the idea is that we'll add more stuff down the road, then see below ... > 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); > + > + last_setup_data_offset = prot_addr + setup_data_offset; if the idea is that we'll add more stuff down the road, then it should be += here. > } > > + setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); > + kernel_size = setup_data_offset + sizeof(struct setup_data) + 32; > + kernel = g_realloc(kernel, kernel_size); > + setup_data = (struct setup_data *)(kernel + setup_data_offset); > + setup_data->next = last_setup_data_offset; Likely broken on LE. > + setup_data->type = cpu_to_le32(SETUP_RNG_SEED); > + setup_data->len = cpu_to_le32(32); > + qemu_guest_getrandom_nofail(setup_data->data, 32); > + > + last_setup_data_offset = prot_addr + setup_data_offset; where does this 32 come from? maybe make it a macro. > + > + stq_p(header + 0x250, last_setup_data_offset); add a comment while we are at it? > + > /* > * If we're starting an encrypted VM, it will be OVMF based, which uses the > * efi stub for booting and doesn't require any values to be placed in the > diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h > index 072e2ed546..b2aaad10e5 100644 > --- a/include/standard-headers/asm-x86/bootparam.h > +++ b/include/standard-headers/asm-x86/bootparam.h > @@ -10,6 +10,7 @@ > #define SETUP_EFI 4 > #define SETUP_APPLE_PROPERTIES 5 > #define SETUP_JAILHOUSE 6 > +#define SETUP_RNG_SEED 9 > > #define SETUP_INDIRECT (1<<31) > > -- > 2.35.1
Hi Michael, Thanks for the feedback. On Thu, Jul 21, 2022 at 06:35:41AM -0400, Michael S. Tsirkin wrote: > > - setup_data->next = 0; > > + setup_data->next = last_setup_data_offset; > > does this make any difference? if the idea is that we'll add more stuff > down the road, then see below ... It doesn't; it's just for completeness, in case somebody decides to add something prior, and then less code has to change and there's less chance of an error. The compiler generates the same code either way. > > > 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); > > + > > + last_setup_data_offset = prot_addr + setup_data_offset; > > > if the idea is that we'll add more stuff down the road, then > it should be += here. It's just poorly named actually. It should be called "prev_setup_data_prot_addr" or something. I'll find a better name for v+1. > > > } > > > > + setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); > > + kernel_size = setup_data_offset + sizeof(struct setup_data) + 32; > > + kernel = g_realloc(kernel, kernel_size); > > + setup_data = (struct setup_data *)(kernel + setup_data_offset); > > + setup_data->next = last_setup_data_offset; > > Likely broken on LE. Nice catch, thanks. > > > + setup_data->type = cpu_to_le32(SETUP_RNG_SEED); > > + setup_data->len = cpu_to_le32(32); > > + qemu_guest_getrandom_nofail(setup_data->data, 32); > > + > > + last_setup_data_offset = prot_addr + setup_data_offset; > > > where does this 32 come from? maybe make it a macro. Will do. > > > + > > + stq_p(header + 0x250, last_setup_data_offset); > > add a comment while we are at it? Ack. Jason
diff --git a/hw/i386/x86.c b/hw/i386/x86.c index 6003b4b2df..284c97f158 100644 --- a/hw/i386/x86.c +++ b/hw/i386/x86.c @@ -26,6 +26,7 @@ #include "qemu/cutils.h" #include "qemu/units.h" #include "qemu/datadir.h" +#include "qemu/guest-random.h" #include "qapi/error.h" #include "qapi/qmp/qerror.h" #include "qapi/qapi-visit-common.h" @@ -771,7 +772,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, setup_data_offset; + int dtb_size, setup_data_offset, last_setup_data_offset = 0; uint32_t initrd_max; uint8_t header[8192], *setup, *kernel; hwaddr real_addr, prot_addr, cmdline_addr, initrd_addr = 0; @@ -1063,16 +1064,30 @@ void x86_load_linux(X86MachineState *x86ms, kernel_size = setup_data_offset + sizeof(struct setup_data) + dtb_size; kernel = g_realloc(kernel, kernel_size); - stq_p(header + 0x250, prot_addr + setup_data_offset); setup_data = (struct setup_data *)(kernel + setup_data_offset); - setup_data->next = 0; + setup_data->next = last_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); + + last_setup_data_offset = prot_addr + setup_data_offset; } + setup_data_offset = QEMU_ALIGN_UP(kernel_size, 16); + kernel_size = setup_data_offset + sizeof(struct setup_data) + 32; + kernel = g_realloc(kernel, kernel_size); + setup_data = (struct setup_data *)(kernel + setup_data_offset); + setup_data->next = last_setup_data_offset; + setup_data->type = cpu_to_le32(SETUP_RNG_SEED); + setup_data->len = cpu_to_le32(32); + qemu_guest_getrandom_nofail(setup_data->data, 32); + + last_setup_data_offset = prot_addr + setup_data_offset; + + stq_p(header + 0x250, last_setup_data_offset); + /* * If we're starting an encrypted VM, it will be OVMF based, which uses the * efi stub for booting and doesn't require any values to be placed in the diff --git a/include/standard-headers/asm-x86/bootparam.h b/include/standard-headers/asm-x86/bootparam.h index 072e2ed546..b2aaad10e5 100644 --- a/include/standard-headers/asm-x86/bootparam.h +++ b/include/standard-headers/asm-x86/bootparam.h @@ -10,6 +10,7 @@ #define SETUP_EFI 4 #define SETUP_APPLE_PROPERTIES 5 #define SETUP_JAILHOUSE 6 +#define SETUP_RNG_SEED 9 #define SETUP_INDIRECT (1<<31)
Tiny machines optimized for fast boot time generally don't use EFI, which means a random seed has to be supplied some other way. For this purpose, Linux (≥5.20) supports passing a seed in the setup_data table with SETUP_RNG_SEED, specially intended for hypervisors, kexec, and specialized bootloaders. The linked commit shows the upstream kernel implementation. Link: https://git.kernel.org/tip/tip/c/68b8e9713c8 Cc: Michael S. Tsirkin <mst@redhat.com> Cc: Marcel Apfelbaum <marcel.apfelbaum@gmail.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Eduardo Habkost <eduardo@habkost.net> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org> Cc: Laurent Vivier <laurent@vivier.eu> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com> --- hw/i386/x86.c | 21 +++++++++++++++++--- include/standard-headers/asm-x86/bootparam.h | 1 + 2 files changed, 19 insertions(+), 3 deletions(-)