diff mbox series

x86: re-initialize RNG seed when selecting kernel

Message ID 20220922152847.3670513-1-Jason@zx2c4.com
State New
Headers show
Series x86: re-initialize RNG seed when selecting kernel | expand

Commit Message

Jason A. Donenfeld Sept. 22, 2022, 3:28 p.m. UTC
We don't want it to be possible to re-read the RNG seed after ingesting
it, because this ruins forward secrecy. Currently, however, the setup
data section can just be re-read. Since the kernel is always read after
the setup data, use the selection of the kernel as a trigger to
re-initialize the RNG seed, just like we do on reboot, to preserve
forward secrecy.

Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
---
Paolo- this applies on top of the 4 you merged this morning. -Jason

 hw/i386/x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Jason A. Donenfeld Sept. 26, 2022, 1:39 p.m. UTC | #1
On Thu, Sep 22, 2022 at 5:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> We don't want it to be possible to re-read the RNG seed after ingesting
> it, because this ruins forward secrecy. Currently, however, the setup
> data section can just be re-read. Since the kernel is always read after
> the setup data, use the selection of the kernel as a trigger to
> re-initialize the RNG seed, just like we do on reboot, to preserve
> forward secrecy.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> ---
> Paolo- this applies on top of the 4 you merged this morning. -Jason

Just bumping this, in hopes that this can go out with the same PULL
for the other 4 you merged last week.

Jason
Paolo Bonzini Sept. 26, 2022, 4:07 p.m. UTC | #2
On Mon, Sep 26, 2022 at 3:45 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Thu, Sep 22, 2022 at 5:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > We don't want it to be possible to re-read the RNG seed after ingesting
> > it, because this ruins forward secrecy. Currently, however, the setup
> > data section can just be re-read. Since the kernel is always read after
> > the setup data, use the selection of the kernel as a trigger to
> > re-initialize the RNG seed, just like we do on reboot, to preserve
> > forward secrecy.
> >
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > ---
> > Paolo- this applies on top of the 4 you merged this morning. -Jason
>
> Just bumping this, in hopes that this can go out with the same PULL
> for the other 4 you merged last week.

Thanks, queued but I have a question.

If I understand correctly, this protects against rereading the seed while the
OS is running. If so, does that mean that the device tree-based seed
initialization does not have forward secrecy at all?

Paolo
Jason A. Donenfeld Sept. 26, 2022, 4:18 p.m. UTC | #3
Hi Paolo,

On Mon, Sep 26, 2022 at 06:07:43PM +0200, Paolo Bonzini wrote:
> On Mon, Sep 26, 2022 at 3:45 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On Thu, Sep 22, 2022 at 5:28 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > > We don't want it to be possible to re-read the RNG seed after ingesting
> > > it, because this ruins forward secrecy. Currently, however, the setup
> > > data section can just be re-read. Since the kernel is always read after
> > > the setup data, use the selection of the kernel as a trigger to
> > > re-initialize the RNG seed, just like we do on reboot, to preserve
> > > forward secrecy.
> > >
> > > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > > Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
> > > ---
> > > Paolo- this applies on top of the 4 you merged this morning. -Jason
> >
> > Just bumping this, in hopes that this can go out with the same PULL
> > for the other 4 you merged last week.
> 
> Thanks, queued but I have a question.

Thank you. Also hope to see pc-bios/qboot.rom rebuilt based on that new
submodule commit. QEMU uses the prebuilt in its tree, rather than
rebuilding from the submodule. (I sent a patch earlier rebuilding
qboot.rom in the tree, but you should probably do this yourself because
it's pretty darn hard to verify binary diffs like that on the mailing
list.)

> If I understand correctly, this protects against rereading the seed while the
> OS is running. If so, does that mean that the device tree-based seed
> initialization does not have forward secrecy at all?

On both x86 and dtb-based archs, the seed in memory is zeroed out by the
kernel after reading. So, as far as the guest is concerned, there's
forward secrecy. Except! Except if the guest has someway of
re-requesting that seed from the host. This patch prevents that from
happening through fw_cfg on x86. Somebody told me last week that device
tree archs don't use fw_cfg, so this won't be a problem there. I haven't
yet looked to verify that yet, though, or looked if there are other
mechanisms.

Jason
Peter Maydell Sept. 26, 2022, 5:05 p.m. UTC | #4
On Mon, 26 Sept 2022 at 17:53, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On both x86 and dtb-based archs, the seed in memory is zeroed out by the
> kernel after reading. So, as far as the guest is concerned, there's
> forward secrecy. Except! Except if the guest has someway of
> re-requesting that seed from the host. This patch prevents that from
> happening through fw_cfg on x86. Somebody told me last week that device
> tree archs don't use fw_cfg, so this won't be a problem there. I haven't
> yet looked to verify that yet, though, or looked if there are other
> mechanisms.

I am leaping in here with no context, so I may well have
the wrong end of the stick, but:

"does this system have a fw_cfg device" and "does this system
pass a device tree to the kernel" are orthogonal questions:

 fw_cfg, no device tree: classic x86 pc; arm virt board when
   booting UEFI firmware in the guest
 fw_cfg, device tree: arm virt board booting a kernel directly
 no fw_cfg, device tree: arm vexpress-a15 board (or any other
   just-emulating-hardware machine type)
 no fw_cfg, no device tree: arm sbsa-ref board (and probably
   lots of non-arm architecture machines too)

PS: if we're going to give FW_CFG_KERNEL_DATA magic side
effect behaviour, is there somewhere we can document that magic?

thanks
-- PMM
Jason A. Donenfeld Sept. 26, 2022, 5:08 p.m. UTC | #5
Hi Peter,

On Mon, Sep 26, 2022 at 7:05 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Mon, 26 Sept 2022 at 17:53, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> > On both x86 and dtb-based archs, the seed in memory is zeroed out by the
> > kernel after reading. So, as far as the guest is concerned, there's
> > forward secrecy. Except! Except if the guest has someway of
> > re-requesting that seed from the host. This patch prevents that from
> > happening through fw_cfg on x86. Somebody told me last week that device
> > tree archs don't use fw_cfg, so this won't be a problem there. I haven't
> > yet looked to verify that yet, though, or looked if there are other
> > mechanisms.
>
> I am leaping in here with no context, so I may well have
> the wrong end of the stick, but:
>
> "does this system have a fw_cfg device" and "does this system
> pass a device tree to the kernel" are orthogonal questions:
>
>  fw_cfg, no device tree: classic x86 pc; arm virt board when
>    booting UEFI firmware in the guest
>  fw_cfg, device tree: arm virt board booting a kernel directly
>  no fw_cfg, device tree: arm vexpress-a15 board (or any other
>    just-emulating-hardware machine type)
>  no fw_cfg, no device tree: arm sbsa-ref board (and probably
>    lots of non-arm architecture machines too)

Okay it sounds like I've got to look into this indeed (as my "yet" in
the previous message suggested). Specifically, the case relevant to
this discussion is device tree that goes through fw_cfg. I've got a
few other investigations I'd like to do over there anyway (looking
into how reboots work), so I'll send a series for that when I've got
things worked out.

For the time being, though, this x86 work here is independent of that.
But I suppose you can expect to hear from me not before long about
device tree things.

Jason
diff mbox series

Patch

diff --git a/hw/i386/x86.c b/hw/i386/x86.c
index f9a4ddaa4a..1148f70c03 100644
--- a/hw/i386/x86.c
+++ b/hw/i386/x86.c
@@ -1112,11 +1112,14 @@  void x86_load_linux(X86MachineState *x86ms,
         setup_data->len = cpu_to_le32(RNG_SEED_LENGTH);
         qemu_guest_getrandom_nofail(setup_data->data, RNG_SEED_LENGTH);
         qemu_register_reset(reset_rng_seed, setup_data);
+        fw_cfg_add_bytes_callback(fw_cfg, FW_CFG_KERNEL_DATA, reset_rng_seed, NULL,
+                                  setup_data, kernel, kernel_size, true);
+    } else {
+        fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
     }
 
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_ADDR, prot_addr);
     fw_cfg_add_i32(fw_cfg, FW_CFG_KERNEL_SIZE, kernel_size);
-    fw_cfg_add_bytes(fw_cfg, FW_CFG_KERNEL_DATA, kernel, kernel_size);
     sev_load_ctx.kernel_data = (char *)kernel;
     sev_load_ctx.kernel_size = kernel_size;