[libvirt] [PATCH v2] qemu: snapshot: Forbid internal snapshots with pflash firmware

[Peter Krempa]

On Thu, Mar 23, 2017 at 17:49:56 +0100, Laszlo Ersek wrote:
> On 03/23/17 15:07, Peter Krempa wrote:
> > On Thu, Mar 23, 2017 at 11:03:02 +0100, Laszlo Ersek wrote:
> >> On 03/23/17 10:54, Peter Krempa wrote:
> >>> On Thu, Mar 23, 2017 at 10:48:01 +0100, Laszlo Ersek wrote:
> >>>> On 03/23/17 10:29, Peter Krempa wrote:

[...]

> > Due to the facts above I think that there are users that legitimately
> > think that snapshots with pflash loaders work as expected. It's mostly
> > due to the fact that the data are pretty static and OSes don't store
> > anything important there and are able to self-heal some of the problems.
> > 
> > I think we should not disallow this to avoid usability regressions. We
> > can add documentation that states that it's unsafe to do snapshots.
> > Additionally we will need to add support for external snapshots, which
> > currently have similar kind of problems, although fixable.
> 
> The tradeoff is between a seemingly working, but inherently unsafe
> operation, and a clear error message that keeps things safe.
> 
> The UEFI variable store is used for more and different things than you
> mention, such as (in roughly decreasing order of importance):
> 
> * Some UEFI variables (the authenticated ones) have security impact.
> This covers the standardized ones used for Secure Boot (Platform Key,
> Key Exchange Keys, white-listed certificates and signatures (DB) and
> black-listed certificates and signatures (DBX)).
> 
> * To my knowledge, it also includes some similar security-related
> variables used by shim / MokManager (where "MOK" is short for Machine
> Owner Key); that is, non-volatile variables to which shim delegates the
> EFI binaries' verification, from the standardized Secure Boot interfaces.
> 
> * UEFI variables can serve as the backend for the linux "pstore"
> (persistent store) file system. Pstore in turn can be used to save the
> last part of dmesg on a crash. The messages can be re-read at a new boot.
> 
> * Firmware uses (reads/writes) a number of variables internally at each
> boot. These may not be critical. One example is a variable that helps
> reduce UEFI memory map fragmentation over a series of boots.
> 
> * OVMF manipulates UEFI boot options on each boot, according to the
> bootindex properties (or more directly, according to the "bootorder"
> fw_cfg file). Although, admittedly, this is likely the least risky
> category of varstore contents.

I was worried about the secure boot case too. Thanks for pointing out
that OVMF actually uses other variables too.

> While I have myself successfully used -- offline only -- internal
> snapshots with OVMF guests, hand-waving away the knowledge that the
> varstore was never actually snapshotted, I feel real uncomfortable about
> silently performing an inherently lossy operation, especially when the
> varstore may well have security impact.

I agree.

> 
> Users will not read the documentation (they never do), and I would
> rather not field future bug reports about obscure Secure Boot misbehavior.
> 
> It is ultimately up to libvirt developers, but IMO, if we continue to
> allow this unsafe operation, then the minimum would be:
> 
> * in virt-manager, pop up an extra confirmation dialog, with clear
> indication that the operation will be lossy and could have security impact,

I'll file a bug for that ...

> 
> * in virsh, reject the operation with a similar error message, unless
> "--force" or something similar were specified.

and introduce this, called --unsafe in virsh and
VIR_ERR_OPERATION_UNSAFE for notification and
VIR_DOMAIN_SNAPSHOT_CREATE_UNSAFE for overriding.

> 
> * And, because there are other (independent) libvirt client
> applications, this would likely require a new flag on the libvirt API
> level, so that libvirt itself can reject unsafe snapshotting requests,
> regardless of the client application that submits it.
> 
> I agree that usability regressions are not nice and will likely generate
> bug reports; however, *if* they are in direct conflict with security
> improvements, then security improvements trump usability regressions. I
> guess we can allow users to ignore security, but they need to be
> informed on the spot, and they have to opt in.

Agreed.

> 
> I would prefer if we went ahead with this patch; but, again, it's up to
> you in the end.

I'll post a v3 with the option to override it, if users insist that they
don't care about the state of their varstore.

Peter
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170324/42159040/attachment-0001.sig>