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

[Laszlo Ersek]

On 03/24/17 13:46, Peter Krempa wrote:
> 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.

Thank you, this all sounds good to me.

Could you please test v3 with both live (online) and offline OVMF VMs,
with and without --unsafe? (That is, four cases in total.)

Maybe that goes without saying :), but I figured I would ask for full
testing coverage explicitly, because from
<https://bugzilla.redhat.com/show_bug.cgi?id=1214187> it is not exactly
clear to me whether right now we'd have an actual functional failure for
live OVMF VMs, even with --unsafe.

Thanks!
Laszlo