[PATCH RFC 0/7] revert RNG seed mess

Michael S. Tsirkin posted 7 patches 1 year, 2 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20230208211212.41951-1-mst@redhat.com
Maintainers: Sergio Lopez <slp@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Richard Henderson <richard.henderson@linaro.org>, Eduardo Habkost <eduardo@habkost.net>, "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Gerd Hoffmann <kraxel@redhat.com>
include/hw/i386/microvm.h |   5 +-
include/hw/i386/pc.h      |   3 -
include/hw/i386/x86.h     |   3 +-
include/hw/nvram/fw_cfg.h |  31 ----------
hw/i386/microvm.c         |  17 ++----
hw/i386/pc.c              |   4 +-
hw/i386/pc_piix.c         |   2 -
hw/i386/pc_q35.c          |   2 -
hw/i386/x86.c             | 122 ++++++++++----------------------------
hw/nvram/fw_cfg.c         |  21 ++-----
10 files changed, 49 insertions(+), 161 deletions(-)
[PATCH RFC 0/7] revert RNG seed mess
Posted by Michael S. Tsirkin 1 year, 2 months ago
All attempts to fix up passing RNG seed via setup_data entry failed.
Let's just rip out all of it.  We'll start over.


Warning: all I did was git revert the relevant patches and resolve the
(trivial) conflicts. Not even compiled - it's almost midnight here.

Jason this is the kind of approach I'd like to see, not yet another
pointer math rich patch I need to spend time reviewing. Just get us back
to where we started. We can redo "x86: use typedef for SetupData struct"
later if we want, it's benign.

Could you do something like this pls?
Or test and ack if this patchset happens to work by luck.

Michael S. Tsirkin (7):
  Revert "x86: don't let decompressed kernel image clobber setup_data"
  Revert "x86: do not re-randomize RNG seed on snapshot load"
  Revert "x86: re-initialize RNG seed when selecting kernel"
  Revert "x86: reinitialize RNG seed on system reboot"
  Revert "x86: use typedef for SetupData struct"
  Revert "x86: return modified setup_data only if read as memory, not as
    file"
  Revert "hw/i386: pass RNG seed via setup_data entry"

 include/hw/i386/microvm.h |   5 +-
 include/hw/i386/pc.h      |   3 -
 include/hw/i386/x86.h     |   3 +-
 include/hw/nvram/fw_cfg.h |  31 ----------
 hw/i386/microvm.c         |  17 ++----
 hw/i386/pc.c              |   4 +-
 hw/i386/pc_piix.c         |   2 -
 hw/i386/pc_q35.c          |   2 -
 hw/i386/x86.c             | 122 ++++++++++----------------------------
 hw/nvram/fw_cfg.c         |  21 ++-----
 10 files changed, 49 insertions(+), 161 deletions(-)

-- 
MST
Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Dov Murik 1 year, 2 months ago
Hi Michael,

On 08/02/2023 23:12, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.
> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 


I tested this series with SEV measured boot using AmdSev OVMF.  The boot
succeeds, and the hashes computed for kernel/initrd/cmdline are
identical to the ones computed by qemu 7.1.0, which are the hashes that
the guest owner would expect.

As for non-EFI (non-SEV) guests, I did a very simple test of starting a
non-EFI guest and it looks OK, but more testing is needed.

So:

Tested-by: Dov Murik <dovmurik@linux.ibm.com>


Thank you!

-Dov
Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Dov Murik 1 year, 2 months ago

On 09/02/2023 8:03, Dov Murik wrote:
> Hi Michael,
> 
> On 08/02/2023 23:12, Michael S. Tsirkin wrote:
>> All attempts to fix up passing RNG seed via setup_data entry failed.
>> Let's just rip out all of it.  We'll start over.
>>
>>
>> Warning: all I did was git revert the relevant patches and resolve the
>> (trivial) conflicts. Not even compiled - it's almost midnight here.
>>
>> Jason this is the kind of approach I'd like to see, not yet another
>> pointer math rich patch I need to spend time reviewing. Just get us back
>> to where we started. We can redo "x86: use typedef for SetupData struct"
>> later if we want, it's benign.
>>
>> Could you do something like this pls?
>> Or test and ack if this patchset happens to work by luck.
>>
>> Michael S. Tsirkin (7):
>>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>>   Revert "x86: re-initialize RNG seed when selecting kernel"
>>   Revert "x86: reinitialize RNG seed on system reboot"
>>   Revert "x86: use typedef for SetupData struct"
>>   Revert "x86: return modified setup_data only if read as memory, not as
>>     file"
>>   Revert "hw/i386: pass RNG seed via setup_data entry"
>>
>>  include/hw/i386/microvm.h |   5 +-
>>  include/hw/i386/pc.h      |   3 -
>>  include/hw/i386/x86.h     |   3 +-
>>  include/hw/nvram/fw_cfg.h |  31 ----------
>>  hw/i386/microvm.c         |  17 ++----
>>  hw/i386/pc.c              |   4 +-
>>  hw/i386/pc_piix.c         |   2 -
>>  hw/i386/pc_q35.c          |   2 -
>>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>>  hw/nvram/fw_cfg.c         |  21 ++-----
>>  10 files changed, 49 insertions(+), 161 deletions(-)
>>
> 
> 
> I tested this series with SEV measured boot using AmdSev OVMF.  The boot
> succeeds, and the hashes computed for kernel/initrd/cmdline are
> identical to the ones computed by qemu 7.1.0, which are the hashes that
> the guest owner would expect.
> 

I also tested that backporting this series to 7.2.0 (skipping
PATCH 1/7 because it was added after the 7.2.0 release) works OK for
booting SEV guest with AmdSev measured boot (and retains the same
kernel/initrd/cmdline hashes as qemu 7.1.0).

-Dov


> As for non-EFI (non-SEV) guests, I did a very simple test of starting a
> non-EFI guest and it looks OK, but more testing is needed.
> 
> So:
> 
> Tested-by: Dov Murik <dovmurik@linux.ibm.com>
> 
> 
> Thank you!
> 
> -Dov
Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.

This approach looks suitable for applying to the 7.2 tree too,
which will be good for fixing the regressions in stable.

> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 
> -- 
> MST
> 

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Daniel P. Berrangé 1 year, 2 months ago
On Fri, Feb 10, 2023 at 11:32:58AM +0000, Daniel P. Berrangé wrote:
> On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> > All attempts to fix up passing RNG seed via setup_data entry failed.
> > Let's just rip out all of it.  We'll start over.
> > 
> > 
> > Warning: all I did was git revert the relevant patches and resolve the
> > (trivial) conflicts. Not even compiled - it's almost midnight here.
> > 
> > Jason this is the kind of approach I'd like to see, not yet another
> > pointer math rich patch I need to spend time reviewing. Just get us back
> > to where we started. We can redo "x86: use typedef for SetupData struct"
> > later if we want, it's benign.
> 
> This approach looks suitable for applying to the 7.2 tree too,
> which will be good for fixing the regressions in stable.

Since no further alternative has been proposed, can you consider sending
a pull request for this series. This has been broken for too long and
many users & vendors are looking for an official fix to be applied to
master before they backport to 7.2

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|


Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Michael S. Tsirkin 1 year, 2 months ago
On Mon, Feb 20, 2023 at 10:48:57AM +0000, Daniel P. Berrangé wrote:
> On Fri, Feb 10, 2023 at 11:32:58AM +0000, Daniel P. Berrangé wrote:
> > On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> > > All attempts to fix up passing RNG seed via setup_data entry failed.
> > > Let's just rip out all of it.  We'll start over.
> > > 
> > > 
> > > Warning: all I did was git revert the relevant patches and resolve the
> > > (trivial) conflicts. Not even compiled - it's almost midnight here.
> > > 
> > > Jason this is the kind of approach I'd like to see, not yet another
> > > pointer math rich patch I need to spend time reviewing. Just get us back
> > > to where we started. We can redo "x86: use typedef for SetupData struct"
> > > later if we want, it's benign.
> > 
> > This approach looks suitable for applying to the 7.2 tree too,
> > which will be good for fixing the regressions in stable.
> 
> Since no further alternative has been proposed, can you consider sending
> a pull request for this series. This has been broken for too long and
> many users & vendors are looking for an official fix to be applied to
> master before they backport to 7.2
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Will do. Thanks!
Re: [PATCH RFC 0/7] revert RNG seed mess
Posted by Nathan Chancellor 1 year, 2 months ago
On Wed, Feb 08, 2023 at 04:12:23PM -0500, Michael S. Tsirkin wrote:
> All attempts to fix up passing RNG seed via setup_data entry failed.
> Let's just rip out all of it.  We'll start over.
> 
> 
> Warning: all I did was git revert the relevant patches and resolve the
> (trivial) conflicts. Not even compiled - it's almost midnight here.
> 
> Jason this is the kind of approach I'd like to see, not yet another
> pointer math rich patch I need to spend time reviewing. Just get us back
> to where we started. We can redo "x86: use typedef for SetupData struct"
> later if we want, it's benign.
> 
> Could you do something like this pls?
> Or test and ack if this patchset happens to work by luck.
> 
> Michael S. Tsirkin (7):
>   Revert "x86: don't let decompressed kernel image clobber setup_data"
>   Revert "x86: do not re-randomize RNG seed on snapshot load"
>   Revert "x86: re-initialize RNG seed when selecting kernel"
>   Revert "x86: reinitialize RNG seed on system reboot"
>   Revert "x86: use typedef for SetupData struct"
>   Revert "x86: return modified setup_data only if read as memory, not as
>     file"
>   Revert "hw/i386: pass RNG seed via setup_data entry"
> 
>  include/hw/i386/microvm.h |   5 +-
>  include/hw/i386/pc.h      |   3 -
>  include/hw/i386/x86.h     |   3 +-
>  include/hw/nvram/fw_cfg.h |  31 ----------
>  hw/i386/microvm.c         |  17 ++----
>  hw/i386/pc.c              |   4 +-
>  hw/i386/pc_piix.c         |   2 -
>  hw/i386/pc_q35.c          |   2 -
>  hw/i386/x86.c             | 122 ++++++++++----------------------------
>  hw/nvram/fw_cfg.c         |  21 ++-----
>  10 files changed, 49 insertions(+), 161 deletions(-)
> 
> -- 
> MST
> 

For the record, all three of the cases that I tested (i386 no EFI,
x86_64 with and without EFI) worked fine with this series. In case it is
useful:

Tested-by: Nathan Chancellor <nathan@kernel.org>

Cheers,
Nathan